Unify the plugin installation for standalone and context-scoped plugins and use context as recommendation #698
Conversation
anujc25
force-pushed
the
install-context-plugins-as-standalone-plugins-v3
branch
2 times, most recently
from
5d2df69 to
3f7ce7e
Compare
anujc25
force-pushed
the
install-context-plugins-as-standalone-plugins-v3
branch
from
3f7ce7e to
6b87894
Compare
anujc25
force-pushed
the
install-context-plugins-as-standalone-plugins-v3
branch
from
0d4bf7f to
58effbc
Compare
| // Migrate context-scoped plugins as standalone plugin if required | ||
| // TODO(anujc): Think on how to invoke this function just once after the newer version | ||
| // of the CLI gets installed as we just need to do this migration once | ||
| catalog.MigrateContextPluginsAsStandaloneIfNeeded() |
There was a problem hiding this comment.
#723 can be useful for this situation.
anujc25
force-pushed
the
install-context-plugins-as-standalone-plugins-v3
branch
from
ba87e83 to
f34fbc6
Compare
There was a problem hiding this comment.
I like this new approach! Thanks for pushing for this @anujc25.
I'm about half way through the review, but here are some minor comments to get started, since we are pressed for time.
There was a problem hiding this comment.
I finished the review.
I like this. A lot. My first impression as a user is that it makes things much simpler.
Here are my last comments. Again, minor.
Beautiful job, as usual, @anujc25 .
- From the jira, this was not done: "Update
tanzu plugin list --helpto specify the meaning of INSTALLED and RECOMMENDED columns". But to be honest, I think the meaning is pretty clear without needing any explanation, so would leave it alone. - On the other hand, it is only when I saw the code that I understood what the ACTIVE column really meant. Until then I thought it was a short form of
installedvsnot installed. I think this needs to be described, maybe in the help. - The status "update recommended" should be "upgrade recommended" I think. The command we have is
tanzu plugin upgrade. Also, for package managers,updateis used to get the latest info whileupgradeis used to install a new version. - Am I right that in a future effort, we can cleanup the logic of the catalog to no longer deal with context plugins?
- For discussion for a potential follow-up:
I was about to delete this comment, but then I thought that when a user sees "install recommended" or "update recommended" in theplugin listoutput, they may just trytanzu plugin install <plugin>. The below would address this scenario.
What if, when doingplugin install <plugin>with no version specified, instead of always using the latest we would use the recommended version? One could argue that if a version is recommended, that is what should be installed withplugin install. We could also not require the --target flag if only one plugin with a name is recommended but not installed.
Below are some thoughts that I'd like to discuss with you. I'm not sure any of them are good ideas or not.
A- In plugin list, in the RECOMMENDED column, should we put "N/A" or "none" or something like that for plugins that are not currently recommended?
B- In plugin list, I was wondering about the sorting. Would it be nicer to sort uninstalled plugins at the end, or maybe even better sorting by the STATUS colum so plugins that need a sync (to install or upgrade) are shown at the bottom?
C- When synching plugins (plugin sync, context create/use) I find the column names a bit confusing:
$ tz context use tkg3
[i] Activated context 'tkg3' (Type: kubernetes)
[i] Fetching recommended plugins for active context 'tkg3'...
[i] Installing following plugins recommended by the context 'tkg3':
NAME TARGET INSTALLED RECOMMENDED
cluster kubernetes v0.33.1 v0.25.0
[i] Installed plugin 'cluster:v0.25.0' with target 'kubernetes' (from cache)
Does INSTALLED mean what was just installed or what was previously installed? Does RECOMMENDED mean it will be installed or it SHOULD be installed? Not a big problem with the message right below saying "[i] Installed plugin 'cluster:v0.25.0'", however it will become a bigger problem when we use a spinner and that line is erased quickly. How about using "CURRENT" and "INSTALLING"?
D- I'm getting used to the empty INSTALLED column meaning "not installed", but at the beginning it was not totally obvious, especially since we don't use "not installed" but "install recommended" (which I like). So I'm wondering if we should put "none" instead of empty? But I'm not sure at all to be honest.
E- When I unset a context, do I need a way to uninstall the corresponding context plugins? I'm starting to see that keeping them is probably easier to understand for users, but what will they do if they want to remove them once they get rid of a context that is using plugins that have been abandoned? Rare case probably. I think we should consider to eventually (not this PR) provide a way to do a cleanup. Maybe a plugin clean that does not wipe the cache.
There was a problem hiding this comment.
Added another minor comment.
As far as documenting the user facing impact of the changes, besides updating PR description,
- there are some existing md that should be updated (e.g. quickstart.md)
- we should mention that what commands are available to the user is mostly not dependent on whether a context is active or not anymore
- the release notes need to be updated with a short summary of the changes including the previous point
- "docs-impact" label should be added
anujc25
force-pushed
the
install-context-plugins-as-standalone-plugins-v3
branch
from
2c9b537 to
fb57491
Compare
- Update error and warning messages - Add contextName as part of the `tanzu plugin list` for json/yaml output when plugin is recommended - Add comment to the IsPluginActive function - Hide `--wide` flag for `tanzu plugin list` - Update status to `update needed` and `not installed` instead of `update recommended` and `install recommeneded` respectively - Update column names for the to be installed plugins table when running `tanzu plugin sync` and `tanzu context use/create` to show `CURRENT` and `INSTALLING`.
anujc25
force-pushed
the
install-context-plugins-as-standalone-plugins-v3
branch
from
fb57491 to
c1b34e2
Compare
|
Thanks, @marckhouzam @vuil for the review and feedback. I am updating based on what we discussed in our meeting. Adding some details here based on what we discussed during our meeting.
We decided to hide the
Because there is a chance that when switching the context user might want to downgrade a plugin as well, we decided to use
Yes. I will fill an issue to track this.
+1 Good point. I will file an issue to track this enhancement.
We decided to keep it empty as it is.
We decided to keep it sorted by names for now.
Agreed. Updated PR to address this.
We decided to use empty columns for this one.
We might have to think about this but it might not be based on the context but can do something similar in different ways. But we decided to not do anything for this PR. |
anujc25
force-pushed
the
install-context-plugins-as-standalone-plugins-v3
branch
2 times, most recently
from
a6d6769 to
b971e9d
Compare
anujc25
force-pushed
the
install-context-plugins-as-standalone-plugins-v3
branch
from
b971e9d to
bf1a211
Compare
There was a problem hiding this comment.
Minor nits and a env var rename recommendation, feel free to ignore.
lgtm, you might want to wait for @marckhouzam's approval as well.
There was a problem hiding this comment.
Looks good. Thanks for your effort and patience. This will simplify things for users and ourselves.
Summarizing the follow-ups that I noticed:
- remove
ConfigVariableStandaloneOverContextPlugins - use a global initializer to migrate the catalog
- clean up the catalog to no longer support context plugins
tanzu plugin install <plugin>would not install the latest but would install the recommended version if there is one
…ns and use context as recommendation (vmware-tanzu#698) - Unify the plugin installation for standalone and context-scoped plugins - Use context as a recommendation for which plugin/version to install - Updates the tanzu plugin install and tanzu plugin sync experience - Updates the tanzu plugin list output - Updates the tanzu plugin describe output User facing changes: - Once a plugin is installed and plugin-specific commands are available, that plugin is always shown as installed and all plugin-related commands are available irrespective of whether the plugin was installed manually by the user with the tanzu plugin install command (standalone plugin) or it was installed automatically when the user created a context (context-scoped plugin). - Before this change, context-scoped plugins were getting removed once the corresponding context was deleted or unset. - After this change, the plugin's availability is mostly not dependent on whether a context is active or not, and even after the context gets deleted or unset, corresponding plugins are still available to the user to use.
…ns and use context as recommendation (#698) - Unify the plugin installation for standalone and context-scoped plugins - Use context as a recommendation for which plugin/version to install - Updates the tanzu plugin install and tanzu plugin sync experience - Updates the tanzu plugin list output - Updates the tanzu plugin describe output User facing changes: - Once a plugin is installed and plugin-specific commands are available, that plugin is always shown as installed and all plugin-related commands are available irrespective of whether the plugin was installed manually by the user with the tanzu plugin install command (standalone plugin) or it was installed automatically when the user created a context (context-scoped plugin). - Before this change, context-scoped plugins were getting removed once the corresponding context was deleted or unset. - After this change, the plugin's availability is mostly not dependent on whether a context is active or not, and even after the context gets deleted or unset, corresponding plugins are still available to the user to use.
What this PR does / why we need it
tanzu plugin installandtanzu plugin syncexperiencetanzu plugin listoutputtanzu plugin describeoutputUser facing changes:
tanzu plugin installcommand (standalone plugin) or it was installed automatically when the user created a context (context-scoped plugin).Details:
tanzu plugin list:VERSIONtoINSTALLED.RECOMMENDED.RECOMMENDEDis left empty.RECOMMENDEDcolumn will be dynamic and will only be shown if at least one recommended version.STATUScolumn is used to show the installation and recommended status of the plugin considering the active context. Two new status values are addedinstall recommendedandupdate recommended.--wideto view additional columns.ACTIVEcolumn will be added to show if the plugin is active or not.tanzu plugin list --helpto specify the meaning of INSTALLED and RECOMMENDED columns (TODO)tanzu plugin syncSTATUScolumn will be used to reflect the installation status.tanzu plugin syncortanzu context createortanzu context usecommand, a table will be shown with theINSTALLEDandRECOMMENDEDcolumns to show how the plugin is getting updated (install, upgrade, downgrade).Which issue(s) this PR fixes
Fixes #
Describe testing done for PR
tz plugin installcommand and verify withtz plugin listcluster-1context as an active context that recommended some context-scoped plugins. Verify the installation and check the UXprojectplugin and Install a different version of thespaceplugin that is not recommended by the active context. Verify thetz plugin listoutput.tz plugin syncto install recommended version of the context-scoped plugins.tmccontext that recommends additional context-scoped plugins.Release note
Additional information
Special notes for your reviewer