Skip to content
This repository has been archived by the owner on May 31, 2024. It is now read-only.

Integrate Bubbletea Pagination into get execution #473

Merged
merged 7 commits into from
Apr 26, 2024

Conversation

zychen5186
Copy link
Contributor

@zychen5186 zychen5186 commented Apr 23, 2024

TL;DR

Use Bubbletea Pagination to show the get execution list instead of printing 100 rows each time

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

User can use -i or --interactive to trigger Bubbletea interface, for example,

make compile
./bin/flytectl get execution -p my-project -d development -i

please refer to Bubbletea Github page to see what Bubbletea library offers.
Users can use the Bubbletea interface to check the entire list at once by going left and right, instead of showing 100 rows every command and need to specify which page they want to look at in the command line.
image

Tracking Issue

flyteorg/flyte#4440

Follow-up issue

Integrate this functionality into other flytectl get functions.

Signed-off-by: zychen5186 <brianchen5197@gmail.com>

fix: catches existing commands in os.Args

Signed-off-by: zychen5186 <brianchen5197@gmail.com>

fix: restore neccessary codes

Signed-off-by: zychen5186 <brianchen5197@gmail.com>
Signed-off-by: zychen5186 <brianchen5197@gmail.com>
@zychen5186 zychen5186 force-pushed the bubble-tea-pagination branch from fa5d50b to 99c9b49 Compare April 23, 2024 06:32
Signed-off-by: zychen5186 <brianchen5197@gmail.com>

change dot to arabic paging format

Signed-off-by: zychen5186 <brianchen5197@gmail.com>

change var names

Signed-off-by: zychen5186 <brianchen5197@gmail.com>

fix: lint

Signed-off-by: zychen5186 <brianchen5197@gmail.com>
@zychen5186 zychen5186 force-pushed the bubble-tea-pagination branch from 99c9b49 to 44e2676 Compare April 23, 2024 07:10
Copy link
Member

@troychiu troychiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!

pkg/bubbletea/bubbletea_pagination_util.go Outdated Show resolved Hide resolved
pkg/bubbletea/bubbletea_pagination_util.go Outdated Show resolved Hide resolved
@troychiu troychiu mentioned this pull request Apr 23, 2024
8 tasks
Signed-off-by: zychen5186 <brianchen5197@gmail.com>
Signed-off-by: zychen5186 <brianchen5197@gmail.com>

change := to var

Signed-off-by: zychen5186 <brianchen5197@gmail.com>
Signed-off-by: zychen5186 <brianchen5197@gmail.com>
@troychiu troychiu marked this pull request as ready for review April 24, 2024 00:05
@troychiu
Copy link
Member

@katrogan @pmahindrakar-oss could you help review?

kumare3
kumare3 previously approved these changes Apr 24, 2024
Copy link
Contributor

@kumare3 kumare3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for following the pattern

pingsutw
pingsutw previously approved these changes Apr 24, 2024
Signed-off-by: zychen5186 <brianchen5197@gmail.com>
@zychen5186 zychen5186 dismissed stale reviews from pmahindrakar-oss, pingsutw, and kumare3 via d9d6468 April 25, 2024 02:56
@pmahindrakar-oss
Copy link
Contributor

Can we restrict the flag to be used with only list api's and not for modification api's / on usage display that this mode is not supported for the command being used

Comment on lines +163 to +166
if config.GetConfig().Interactive {
bubbletea.Paginator(executionColumns, getCallBack(ctx, cmdCtx))
return nil
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we restrict the flag to be used with only list api's and not for modification api's / on usage display that this mode is not supported for the command being used

Currently, I'm planning to add pagination to only list API functions, and they will check if -i flag is specified, other non-list API functions will not be affected even if the -i flag is set. Do you think we should explicitly inform users that interactive CLI will not be triggered if they use -i on non-list commands?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we will keep adding interactive feature, I think it's fine if the flag is not yet implemented for some apis at this moment. But we should explicitly point out what apis support interactive mode when this is done.

Copy link
Contributor

@pmahindrakar-oss pmahindrakar-oss Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i Agree since we will keep implementing new changes for this mode. We can probably mention the help string that this is only support for get calls and others is no-op. Can be a followup. but this change looks good

@pmahindrakar-oss pmahindrakar-oss merged commit 46c6751 into flyteorg:master Apr 26, 2024
13 checks passed
@zychen5186 zychen5186 deleted the bubble-tea-pagination branch April 26, 2024 03:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants