From fb57491afbfa7b2278a6dfc94aeacb4425323931 Mon Sep 17 00:00:00 2001 From: Anuj Chaudhari Date: Thu, 18 Apr 2024 11:40:28 -0700 Subject: [PATCH] Address review comments - 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`. --- pkg/command/context.go | 12 ++++---- pkg/command/plugin.go | 21 ++++++++++---- pkg/common/constants.go | 12 ++++---- pkg/pluginmanager/manager.go | 29 ------------------- pkg/pluginmanager/manager_test.go | 7 ----- pkg/pluginsupplier/plugin_supplier.go | 11 +++++++ test/e2e/framework/framework_constants.go | 13 ++++----- .../framework/plugin_lifecycle_operations.go | 2 +- .../k8s/plugin_sync_k8s_lifecycle_test.go | 6 ++-- .../tmc/plugin_sync_tmc_lifecycle_test.go | 2 +- 10 files changed, 48 insertions(+), 67 deletions(-) diff --git a/pkg/command/context.go b/pkg/command/context.go index 29e9897d8..1ec5795f7 100644 --- a/pkg/command/context.go +++ b/pkg/command/context.go @@ -275,7 +275,7 @@ func createCtx(cmd *cobra.Command, args []string) (err error) { // Sync all required plugins if err := syncContextPlugins(cmd, ctx.ContextType, ctxName); err != nil { - log.Warningf("unable to automatically sync the plugins recommended by the active context. Please run 'tanzu plugin sync' command to sync plugins manually, error: '%v'", err.Error()) + log.Warningf("unable to automatically sync the plugins recommended by the new context. Please run 'tanzu plugin sync' to sync plugins manually, error: '%v'", err.Error()) } return nil } @@ -306,12 +306,12 @@ func syncContextPlugins(cmd *cobra.Command, contextType configtypes.ContextType, } if len(pluginsNeedToBeInstalled) == 0 { - log.Success("All recommended plugins are already installed.") + log.Success("All recommended plugins are already installed and up-to-date.") return nil } errList := make([]error, 0) - log.Infof("Installing following plugins recommended by the context '%s':", ctxName) + log.Infof("Installing the following plugins recommended by context '%s':", ctxName) displayToBeInstalledPluginsAsTable(plugins, cmd.ErrOrStderr()) for i := range pluginsNeedToBeInstalled { err = pluginmanager.InstallStandalonePlugin(pluginsNeedToBeInstalled[i].Name, pluginsNeedToBeInstalled[i].RecommendedVersion, pluginsNeedToBeInstalled[i].Target) @@ -329,8 +329,8 @@ func syncContextPlugins(cmd *cobra.Command, contextType configtypes.ContextType, // displayToBeInstalledPluginsAsTable takes a list of plugins and displays the plugin info as a table func displayToBeInstalledPluginsAsTable(plugins []discovery.Discovered, writer io.Writer) { - outputPlugins := component.NewOutputWriterWithOptions(writer, outputFormat, []component.OutputWriterOption{}, "Name", "Target", "Installed", "Recommended") - outputPlugins.MarkDynamicKeys("Installed") + outputPlugins := component.NewOutputWriterWithOptions(writer, outputFormat, []component.OutputWriterOption{}, "Name", "Target", "Current", "Installing") + outputPlugins.MarkDynamicKeys("Current") for i := range plugins { if plugins[i].Status == common.PluginStatusNotInstalled || plugins[i].Status == common.PluginStatusUpdateAvailable { outputPlugins.AddRow(plugins[i].Name, plugins[i].Target, plugins[i].InstalledVersion, plugins[i].RecommendedVersion) @@ -1250,7 +1250,7 @@ func useCtx(cmd *cobra.Command, args []string) error { //nolint:gocyclo // Sync all required plugins if err := syncContextPlugins(cmd, ctx.ContextType, ctxName); err != nil { - log.Warningf("unable to automatically sync the plugins recommended by the active context. Please run 'tanzu plugin sync' command to sync plugins manually, error: '%v'", err.Error()) + log.Warningf("unable to automatically sync the plugins recommended by the active context. Please run 'tanzu plugin sync' to sync plugins manually, error: '%v'", err.Error()) } return nil } diff --git a/pkg/command/plugin.go b/pkg/command/plugin.go index 72959492c..8b4305b79 100644 --- a/pkg/command/plugin.go +++ b/pkg/command/plugin.go @@ -69,6 +69,7 @@ func newPluginCmd() *cobra.Command { listPluginCmd.Flags().StringVarP(&outputFormat, "output", "o", "", "Output format (yaml|json|table)") utils.PanicOnErr(listPluginCmd.RegisterFlagCompletionFunc("output", completionGetOutputFormats)) listPluginCmd.Flags().BoolVar(&showAllColumns, "wide", false, "display additional columns for plugins") + utils.PanicOnErr(listPluginCmd.Flags().MarkHidden("wide")) describePluginCmd.Flags().StringVarP(&outputFormat, "output", "o", "", "Output format (yaml|json|table)") utils.PanicOnErr(describePluginCmd.RegisterFlagCompletionFunc("output", completionGetOutputFormats)) @@ -172,6 +173,7 @@ func newDescribePluginCmd() *cobra.Command { defer fmt.Fprintln(cmd.OutOrStdout()) } output := component.NewOutputWriterWithOptions(cmd.OutOrStdout(), outputFormat, []component.OutputWriterOption{}, "name", "version", "status", "target", "description", "installationPath") + if len(args) != 1 { return fmt.Errorf("must provide one plugin name as a positional argument") } @@ -451,9 +453,11 @@ func syncPlugins(cmd *cobra.Command) error { } for contextType, context := range contextMap { - err = syncContextPlugins(cmd, contextType, context.Name) - if err != nil { - errList = append(errList, err) + if strings.TrimSpace(context.Name) != "" { + err = syncContextPlugins(cmd, contextType, context.Name) + if err != nil { + errList = append(errList, err) + } } } return kerrors.NewAggregate(errList) @@ -466,6 +470,7 @@ type pluginListInfo struct { installed string recommended string status string + contextName string // used only to specify which context recommends the plugin active bool } @@ -507,7 +512,7 @@ func displayInstalledPlugins(installedPlugins []cli.PluginInfo, recommendedConte active: pluginsupplier.IsPluginActive(&installedPlugins[index]), } if p.recommended != "" && p.installed != p.recommended { - p.status = common.PluginStatusRecommendUpdate + p.status = "update needed" pluginSyncRequired = true } plugins = append(plugins, p) @@ -520,7 +525,8 @@ func displayInstalledPlugins(installedPlugins []cli.PluginInfo, recommendedConte target: string(recommendedContextPlugins[index].Target), installed: "", recommended: recommendedContextPlugins[index].RecommendedVersion, - status: common.PluginStatusRecommendInstall, + status: common.PluginStatusNotInstalled, + contextName: recommendedContextPlugins[index].ContextName, active: false, } plugins = append(plugins, p) @@ -539,12 +545,15 @@ func displayInstalledPlugins(installedPlugins []cli.PluginInfo, recommendedConte outputPluginWriter.SetKeys(columnsNames...) outputPluginWriter.MarkDynamicKeys("Recommended") // Marking this column as dynamic so that it will only be shown if at least one row is non-empty for index := range plugins { + // Output writer will ignore and not show additional row data if the row has more data compared to defined keys(column headers). + // So in this case if showAllColumns=false than last value for row (plugins[index].active) will not be shown. So it is safe to provide + // all values to the Row. outputPluginWriter.AddRow(plugins[index].name, plugins[index].description, plugins[index].target, plugins[index].installed, plugins[index].recommended, plugins[index].status, plugins[index].active) } } else { outputPluginWriter.SetKeys("Name", "Description", "Target", "Installed", "Recommended", "Status", "Active", "Context", "Version") // Add 'Context' and 'Version' fields for backwards compatibility for index := range plugins { - outputPluginWriter.AddRow(plugins[index].name, plugins[index].description, plugins[index].target, plugins[index].installed, plugins[index].recommended, plugins[index].status, plugins[index].active, "", plugins[index].installed) + outputPluginWriter.AddRow(plugins[index].name, plugins[index].description, plugins[index].target, plugins[index].installed, plugins[index].recommended, plugins[index].status, plugins[index].active, plugins[index].contextName, plugins[index].installed) } } diff --git a/pkg/common/constants.go b/pkg/common/constants.go index 8bba39fe8..f4da82461 100644 --- a/pkg/common/constants.go +++ b/pkg/common/constants.go @@ -6,13 +6,11 @@ package common // Plugin status and scope constants const ( - PluginStatusInstalled = "installed" - PluginStatusNotInstalled = "not installed" - PluginStatusRecommendInstall = "install recommended" - PluginStatusRecommendUpdate = "update recommended" - PluginStatusUpdateAvailable = "update available" - PluginScopeStandalone = "Standalone" - PluginScopeContext = "Context" + PluginStatusInstalled = "installed" + PluginStatusNotInstalled = "not installed" + PluginStatusUpdateAvailable = "update available" + PluginScopeStandalone = "Standalone" + PluginScopeContext = "Context" ) // DiscoveryType constants diff --git a/pkg/pluginmanager/manager.go b/pkg/pluginmanager/manager.go index 3e2c17f48..30b62f383 100644 --- a/pkg/pluginmanager/manager.go +++ b/pkg/pluginmanager/manager.go @@ -1099,35 +1099,6 @@ func doDeletePluginsFromCatalog(plugins []cli.PluginInfo) error { return kerrors.NewAggregate(errList) } -// SyncPlugins will install the plugins required by the current contexts. -// If the central-repo is disabled, all discovered plugins will be installed. -func SyncPlugins() error { - log.Info("Checking for required plugins...") - errList := make([]error, 0) - // We no longer sync standalone plugins. - // With a centralized approach to discovering plugins, synchronizing - // standalone plugins would install ALL plugins available for ALL - // products. - // Instead, we only synchronize any plugins that are specifically specified - // by the contexts. - // - // Note: to install all plugins for a specific product, plugin groups will - // need to be used. - plugins, err := DiscoverServerPlugins() - if err != nil { - errList = append(errList, err) - } - - for i := range plugins { - err = InstallStandalonePlugin(plugins[i].Name, plugins[i].RecommendedVersion, plugins[i].Target) - if err != nil { - errList = append(errList, err) - } - } - - return kerrors.NewAggregate(errList) -} - func DiscoverPluginsForContextType(contextType configtypes.ContextType) ([]discovery.Discovered, error) { ctx, err := configlib.GetActiveContext(contextType) if err != nil { diff --git a/pkg/pluginmanager/manager_test.go b/pkg/pluginmanager/manager_test.go index 4798ce96f..88cae94c7 100644 --- a/pkg/pluginmanager/manager_test.go +++ b/pkg/pluginmanager/manager_test.go @@ -756,13 +756,6 @@ func Test_SyncPlugins(t *testing.T) { assertions.NotNil(p) assertions.Equal(common.PluginStatusNotInstalled, p.Status) } - - // Sync all available plugins - err = SyncPlugins() - assertions.NotNil(err) - // There is an error for the kubernetes discovery since we don't have a cluster - // but other server plugins will be found, so we use those - assertions.Contains(err.Error(), `Failed to load Kubeconfig file from "config"`) } func Test_setAvailablePluginsStatus(t *testing.T) { diff --git a/pkg/pluginsupplier/plugin_supplier.go b/pkg/pluginsupplier/plugin_supplier.go index bf151d3c3..ac5881115 100644 --- a/pkg/pluginsupplier/plugin_supplier.go +++ b/pkg/pluginsupplier/plugin_supplier.go @@ -54,6 +54,17 @@ func FilterPluginsByActiveContextType(plugins []cli.PluginInfo) (result []cli.Pl } // IsPluginActive returns true if specified plugin is active +// +// Plugin being activated means it is available as Tanzu CLI commands. +// Some plugins are activated conditionally based on a the plugin descriptor. +// If plugin is not active, even if the plugin is installed, it is not +// available as the Tanzu CLI commands for users to use. +// +// For example: If PluginDescription.SupportedContextType is +// specified for the plugin, the plugin is only active +// if the SupportedContextType mentioned with PluginDescription is +// active in Tanzu CLI contexts. +// Reference: https://github.com/vmware-tanzu/tanzu-plugin-runtime/blob/main/plugin/types.go#L117 func IsPluginActive(pi *cli.PluginInfo) bool { if len(pi.SupportedContextType) == 0 { return true diff --git a/test/e2e/framework/framework_constants.go b/test/e2e/framework/framework_constants.go index b28529006..9e4cbb358 100644 --- a/test/e2e/framework/framework_constants.go +++ b/test/e2e/framework/framework_constants.go @@ -80,13 +80,12 @@ const ( CLIE2ETestEnvironment = "TANZU_CLI_E2E_TEST_ENVIRONMENT" // General constants - True = "true" - Installed = "installed" - UpdateAvailable = "update available" - NotInstalled = "not installed" - RecommendInstall = "install recommended" - RecommendUpdate = "update recommended" - JSONOtuput = "-o json" + True = "true" + Installed = "installed" + UpdateAvailable = "update available" + NotInstalled = "not installed" + UpdateNeeded = "update needed" + JSONOtuput = "-o json" // Context commands CreateContextWithEndPoint = "%s context create --endpoint %s %s" diff --git a/test/e2e/framework/plugin_lifecycle_operations.go b/test/e2e/framework/plugin_lifecycle_operations.go index e64e773d4..05abd6adc 100644 --- a/test/e2e/framework/plugin_lifecycle_operations.go +++ b/test/e2e/framework/plugin_lifecycle_operations.go @@ -156,7 +156,7 @@ func (po *pluginCmdOps) ListRecommendedPluginsFromActiveContext(installedOnly bo for i := range plugins { if plugins[i].Recommended != "" { if installedOnly { - if plugins[i].Status == Installed || plugins[i].Status == UpdateAvailable || plugins[i].Status == RecommendUpdate { + if plugins[i].Status == Installed || plugins[i].Status == UpdateAvailable || plugins[i].Status == UpdateNeeded { recommendedPlugins = append(recommendedPlugins, plugins[i]) } } else { diff --git a/test/e2e/plugin_sync/k8s/plugin_sync_k8s_lifecycle_test.go b/test/e2e/plugin_sync/k8s/plugin_sync_k8s_lifecycle_test.go index 16357b3e9..32ec3875e 100644 --- a/test/e2e/plugin_sync/k8s/plugin_sync_k8s_lifecycle_test.go +++ b/test/e2e/plugin_sync/k8s/plugin_sync_k8s_lifecycle_test.go @@ -180,7 +180,7 @@ var _ = f.CLICoreDescribe("[Tests:E2E][Feature:Plugin-sync-lifecycle]", func() { recommendedInstalledPlugins, err = tf.PluginCmd.ListRecommendedPluginsFromActiveContext(true) Expect(err).To(BeNil(), "should not get any error for plugin list") for i := range recommendedInstalledPlugins { - Expect(recommendedInstalledPlugins[i].Status).To(Equal(f.RecommendUpdate), "all installed context-scoped plugin status should show 'update recommended'") + Expect(recommendedInstalledPlugins[i].Status).To(Equal(f.UpdateNeeded), "all installed context-scoped plugin status should show 'update needed'") } _, _, err = tf.PluginCmd.Sync() @@ -207,7 +207,7 @@ var _ = f.CLICoreDescribe("[Tests:E2E][Feature:Plugin-sync-lifecycle]", func() { Expect(err).To(BeNil(), "should not get any error for plugin list") uninstalledPluginInfo := f.GetGivenPluginFromTheGivenPluginList(recommendedPluginsList, pluginToUninstall) - Expect(uninstalledPluginInfo.Status).To(Equal(f.RecommendInstall), "uninstalled plugin should be listed as 'install recommended'") + Expect(uninstalledPluginInfo.Status).To(Equal(f.NotInstalled), "uninstalled plugin should be listed as 'not installed'") Expect(uninstalledPluginInfo.Recommended).To(Equal(pluginToUninstall.Version), "uninstalled plugin should also specify correct recommended column") _, _, err = tf.PluginCmd.Sync() @@ -317,7 +317,7 @@ var _ = f.CLICoreDescribe("[Tests:E2E][Feature:Plugin-sync-lifecycle]", func() { const ( ContextActivated = "Successfully activated context '%s'" PluginWillBeInstalled = "Installing following plugins recommended by the context '%s':" - PluginsTableHeaderRegExp = "NAME\\s+TARGET\\s+RECOMMENDED" + PluginsTableHeaderRegExp = "NAME\\s+TARGET\\s+INSTALLING" PluginsRow = "%s\\s+%s\\s+%s" PluginInstalledRegExp = "Installed plugin '%s:.+' with target '%s'|Reinitialized plugin '%s:.+' with target '%s'" ) diff --git a/test/e2e/plugin_sync/tmc/plugin_sync_tmc_lifecycle_test.go b/test/e2e/plugin_sync/tmc/plugin_sync_tmc_lifecycle_test.go index eb28f9e37..f937a04ee 100644 --- a/test/e2e/plugin_sync/tmc/plugin_sync_tmc_lifecycle_test.go +++ b/test/e2e/plugin_sync/tmc/plugin_sync_tmc_lifecycle_test.go @@ -189,7 +189,7 @@ var _ = f.CLICoreDescribe("[Tests:E2E][Feature:Plugin-Sync-TMC-lifecycle]", func Expect(err).To(BeNil(), "should not get any error for plugin list") uninstalledPluginInfo := f.GetGivenPluginFromTheGivenPluginList(recommendedPluginsList, pluginToUninstall) - Expect(uninstalledPluginInfo.Status).To(Equal(f.RecommendInstall), "uninstalled plugin should be listed as 'install recommended'") + Expect(uninstalledPluginInfo.Status).To(Equal(f.NotInstalled), "uninstalled plugin should be listed as 'not installed'") Expect(uninstalledPluginInfo.Recommended).To(Equal(pluginToUninstall.Version), "uninstalled plugin should also specify correct recommended column") } else { // fail the test case if at least two plugins are not available in the mock response