Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin installation UX update #703

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chandrareddyp
Copy link
Contributor

@chandrareddyp chandrareddyp commented Mar 14, 2024

What this PR does / why we need it

This PR enhances the plugin installation. After these changes, the plugin installation does not log each successful plugin installation message, but it logs the in-progress installation message. However, it does overwrite the log to display the next plugin installation in-progress message. Additionally, each plugin installation in-progress message includes the [total plugins to install / plugins being installed].
This PR impacts below use cases:

  • Plugin installation (stand-alone and target specific, local source)
  • Plugin sync
  • Plugin context use and create
  • Plugin group installation
  • Plugin upgrade

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

  1. Essential Plugin installation:
    image

image

  1. Specific Plugin installation:
    image
    image

image

image

  1. Install plugins from a plugin group:
    image
    image

image

  1. Plugin sync:
    image

When its terminated (with Ctl+c)
image

Plugin sync completed successfully:
image

  1. Context use:
    When context use being terminated (Ctl+c)
    image
    Context use completed successfully:
    image

  2. Plugin install from local source:
    tp plugin install builder --version v1.3.0-dev --local-source /Users/cpamuluri/tkg/tasks/code/cli-core2/tanzu-cli/artifacts/plugins/darwin/amd64
    image

Release note

The plugin installation UX has been updated to log the total number of plugins to install and the number of plugins being installed as part of the in-progress log message. It also removes the successful installation message for each plugin installation.

Additional information

Special notes for your reviewer

@chandrareddyp chandrareddyp requested a review from a team as a code owner March 14, 2024 17:22
@chandrareddyp chandrareddyp force-pushed the feature/plugin-install-ux branch from 1970cdc to 38c406f Compare March 15, 2024 16:44
@chandrareddyp chandrareddyp changed the title first Plugin installation UX update Mar 15, 2024
Copy link
Contributor

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

Thanks!

When installing from a group, the logic uses a different spinner for each plugin. However, each spinner, when closed, adds an empty line to the output. That does not look good:

tz plugin install --group vmware-tkg/default                                                                                  <gke_dev-adhol>
[i] The following plugins will be installed from plugin group 'vmware-tkg/default:v2.5.0'
  NAME                TARGET      VERSION
  isolated-cluster    global      v0.32.1
  management-cluster  kubernetes  v0.32.1
  package             kubernetes  v0.32.1
  pinniped-auth       global      v0.32.1
  secret              kubernetes  v0.32.0
  telemetry           kubernetes  v0.32.1






[ok] successfully installed all plugins from group 'vmware-tkg/default:v2.5.0'

It becomes even worse when creating a TMC context which install 24 plugins, and therefore adds 24 empty lines to the output.
a) We can either fix the spinner API to allow turning off the extra empty line. We had discussed this before.
b) Or we could use the same spinner and just change its text. Maybe a global spinner can be defined instead of a local one?

@chandrareddyp
Copy link
Contributor Author

Thanks!

When installing from a group, the logic uses a different spinner for each plugin. However, each spinner, when closed, adds an empty line to the output. That does not look good:

tz plugin install --group vmware-tkg/default                                                                                  <gke_dev-adhol>
[i] The following plugins will be installed from plugin group 'vmware-tkg/default:v2.5.0'
  NAME                TARGET      VERSION
  isolated-cluster    global      v0.32.1
  management-cluster  kubernetes  v0.32.1
  package             kubernetes  v0.32.1
  pinniped-auth       global      v0.32.1
  secret              kubernetes  v0.32.0
  telemetry           kubernetes  v0.32.1






[ok] successfully installed all plugins from group 'vmware-tkg/default:v2.5.0'

It becomes even worse when creating a TMC context which install 24 plugins, and therefore adds 24 empty lines to the output. a) We can either fix the spinner API to allow turning off the extra empty line. We had discussed this before. b) Or we could use the same spinner and just change its text. Maybe a global spinner can be defined instead of a local one?

Its been fixed, can you use the vmware-tanzu/tanzu-plugin-runtime#170 runtime changes along with this PR.

@chandrareddyp chandrareddyp force-pushed the feature/plugin-install-ux branch from c02e431 to 19cd85b Compare March 22, 2024 14:09
@chandrareddyp chandrareddyp force-pushed the feature/plugin-install-ux branch 3 times, most recently from 58999db to f7a024f Compare April 16, 2024 21:41
@chandrareddyp chandrareddyp force-pushed the feature/plugin-install-ux branch 5 times, most recently from 186acd5 to 9ff9639 Compare April 25, 2024 16:44
@chandrareddyp chandrareddyp force-pushed the feature/plugin-install-ux branch from 9ff9639 to 704b240 Compare April 30, 2024 16:48
@anujc25
Copy link
Contributor

anujc25 commented Apr 30, 2024

I noticed that we are starting count from [0/1] and [0/5] instead of [1/1] and [1/5].

In the testing done section, when installing specific plugins I see

[0/0] Failed to install plugin ...
0 plugins installed out of 0

This doesn't look right to me. It should be [1/1] and 0 plugins installed out of 1. However, I feel that if we are installing just a single plugin we should not be showing 0 plugins installed out of 1 log as well.

Copy link
Contributor

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

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

tanzu plugin upgrade needs to be handled:

$ tz plugin upgrade secret
| [0/0] Already installed : Plugin 'secret:v0.32.1' with target 'kubernetes' [ok] successfully upgraded plugin 'secret'

pkg/pluginmanager/manager.go Outdated Show resolved Hide resolved
pkg/pluginmanager/manager.go Outdated Show resolved Hide resolved
pkg/pluginmanager/manager.go Outdated Show resolved Hide resolved
pkg/pluginmanager/manager.go Outdated Show resolved Hide resolved
pkg/pluginmanager/manager.go Show resolved Hide resolved
pkg/command/context.go Outdated Show resolved Hide resolved
@chandrareddyp chandrareddyp force-pushed the feature/plugin-install-ux branch from 399c016 to 0876913 Compare May 1, 2024 21:15
@chandrareddyp
Copy link
Contributor Author

I noticed that we are starting count from [0/1] and [0/5] instead of [1/1] and [1/5].

In the testing done section, when installing specific plugins I see

[0/0] Failed to install plugin ...
0 plugins installed out of 0

This doesn't look right to me. It should be [1/1] and 0 plugins installed out of 1. However, I feel that if we are installing just a single plugin we should not be showing 0 plugins installed out of 1 log as well.

Whats the use case? @anujc25

@chandrareddyp chandrareddyp force-pushed the feature/plugin-install-ux branch from d80e419 to 748d359 Compare May 1, 2024 21:36
@marckhouzam
Copy link
Contributor

Related to @anujc25's comment, I still see

| [0/6] Installing plugin 'appsv2:v0.2.0-beta.3' with target 'kubernetes'

This should be [1/6]: we are not installing the 0th plugin out of 6, but the 1st plugin out of 6.

@chandrareddyp I guess your idea was that the first number is the number of plugins already installed? I think it is more common that this number is the step number, so 1/6, 2/6 until 6/6 when we are installing the 6th plugin out of 6.

@chandrareddyp
Copy link
Contributor Author

chandrareddyp commented May 6, 2024

Related to @anujc25's comment, I still see

| [0/6] Installing plugin 'appsv2:v0.2.0-beta.3' with target 'kubernetes'

This should be [1/6]: we are not installing the 0th plugin out of 6, but the 1st plugin out of 6.

@chandrareddyp I guess your idea was that the first number is the number of plugins already installed? I think it is more common that this number is the step number, so 1/6, 2/6 until 6/6 when we are installing the 6th plugin out of 6.

Yes the first number indicates no.of plugins already installed.

I felt, it make sense to indicate the count of the plugin being installed! i have updated the code. Thanks.

@chandrareddyp chandrareddyp force-pushed the feature/plugin-install-ux branch 3 times, most recently from 25a0c7b to 49a1005 Compare May 6, 2024 13:19
PR updates the plugin installation UX,
- As part of plugin installation in-progress message,
  it adds total plugins needs to installed and the number of plugins being installed
  The in-progress message will disappear once the installation
  completes.
  Next plugin in-progress will be shown.
- No log messages will be showed once the plugin installation completes.
- This UX applicable to below use case:
  plugin upgrade
  plugin install (target specific and local source)
  plugin sync
  context use
@chandrareddyp chandrareddyp force-pushed the feature/plugin-install-ux branch from 9a109b0 to 191a96a Compare May 6, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants