vmware-tanzu / tanzu-cli

The Tanzu Core CLI project provides the core functionality of the Tanzu CLI. The CLI is based on a plugin architecture where CLI command functionality can be delivered through independently developed plugin binaries
Apache License 2.0
35 stars 22 forks source link

Plugin installation UX update #703

Open chandrareddyp opened 7 months ago

chandrareddyp commented 7 months ago

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:

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 commented 7 months ago

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 https://github.com/vmware-tanzu/tanzu-plugin-runtime/pull/170 runtime changes along with this PR.

anujc25 commented 6 months ago

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.

chandrareddyp commented 6 months ago

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

marckhouzam commented 6 months ago

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 commented 6 months ago

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.