Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history 
- target command child command should respect SupportedContextType
- update target command visibility after remap

- clean up unit test, added test case to verify above changes
  • Loading branch information
@vuil
vuil committed Mar 5, 2024
1 parent b983398 commit 2781c8f
Show file tree
Hide file tree
Showing 3 changed files with 143 additions and 40 deletions.
9 changes: 5 additions & 4 deletions pkg/cli/plugin_cmd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ import (
// the kubernetes group. Any remap location that points to one should also map
// to the other to be consistent.
// This function returns the alternate location, if applicable.
func alternateRemapLocation(p *PluginInfo, location string) string {
func alternateRemapLocation(p *PluginInfo, remapLocation string) string {
if p.Target == configtypes.TargetK8s {
cmdHierarchy := strings.Split(strings.TrimSpace(location), " ")
cmdHierarchy := strings.Split(remapLocation, " ")
if len(cmdHierarchy) == 1 {
return fmt.Sprintf("kubernetes %s", cmdHierarchy[0])
}
Expand All @@ -39,8 +39,9 @@ func alternateRemapLocation(p *PluginInfo, location string) string {
func GetCommandMapForPlugin(p *PluginInfo) map[string]*cobra.Command {
cmdMap := map[string]*cobra.Command{}

for _, remapLocation := range p.InvokedAs {
cmdHierarchy := strings.Split(strings.TrimSpace(remapLocation), " ")
for _, invokedAsItem := range p.InvokedAs {
remapLocation := strings.TrimSpace(invokedAsItem)
cmdHierarchy := strings.Split(remapLocation, " ")

if len(cmdHierarchy) > 0 {
cmdMap[remapLocation] = getCmdForPluginEx(p, cmdHierarchy[len(cmdHierarchy)-1])
Expand Down
37 changes: 25 additions & 12 deletions pkg/command/root.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ func NewRootCmd() (*cobra.Command, error) { //nolint: gocyclo,funlen
// We therefore replace the existing command with the new command by removing the old and
// adding the new one.
maskedPluginsWithPluginOverlap = append(maskedPluginsWithPluginOverlap, matchedCmd.Name())

rootCmd.RemoveCommand(matchedCmd)
rootCmd.AddCommand(cmd)
} else if plugins[i].Name != "login" {
Expand All @@ -124,6 +125,7 @@ func NewRootCmd() (*cobra.Command, error) { //nolint: gocyclo,funlen
}

remapCommandTree(rootCmd, plugins)
updateTargetCommandGroupVisibility()

if len(maskedPluginsWithPluginOverlap) > 0 {
catalog.DeleteIncorrectPluginEntriesFromCatalog()
Expand All @@ -144,7 +146,7 @@ func remapCommandTree(rootCmd *cobra.Command, plugins []cli.PluginInfo) {
for pathKey, cmd := range cmdMap {
matchedCmd, parentCmd := findSubCommandByPath(rootCmd, pathKey)

if parentCmd != nil && parentCmd.Annotations != nil && parentCmd.Annotations["type"] == common.CommandTypePlugin {
if parentCmd != nil && isPluginCommand(parentCmd) {
fmt.Fprintf(os.Stderr, "Remap of plugin into command tree (%s) associated with another plugin is not supported\n", parentCmd.Name())
continue
}
Expand All @@ -156,8 +158,10 @@ func remapCommandTree(rootCmd *cobra.Command, plugins []cli.PluginInfo) {
fmt.Fprintf(os.Stderr, "Unable to remap %s at %q\n", cmd.Name(), pathKey)
}
} else {
parentCmd.RemoveCommand(matchedCmd)
parentCmd.AddCommand(cmd)
if parentCmd != nil {
parentCmd.RemoveCommand(matchedCmd)
parentCmd.AddCommand(cmd)
}
}
}
}
Expand Down Expand Up @@ -186,6 +190,16 @@ func buildReplacementMap(plugins []cli.PluginInfo) map[string]*cobra.Command {
return result
}

// updateTargetCommandGroupVisibility hides commands associated with target
// command group if latter did not acquire any child commands
func updateTargetCommandGroupVisibility() {
for _, targetCmd := range []*cobra.Command{k8sCmd, tmcCmd, opsCmd} {
if len(targetCmd.Commands()) == 0 {
targetCmd.Hidden = true
}
}
}

// setupTargetPlugins sets up the commands for the plugins under the k8s and tmc targets
func setupTargetPlugins() error {
mapTargetToCmd := map[configtypes.Target]*cobra.Command{
Expand All @@ -199,22 +213,21 @@ func setupTargetPlugins() error {
return fmt.Errorf("unable to find installed plugins: %w", err)
}

plugins, err := pluginsupplier.FilterPluginsByActiveContextType(installedPlugins)
if err != nil {
return err
}

// Insert the plugin commands under the appropriate target command
for i := range installedPlugins {
if targetCmd, exists := mapTargetToCmd[installedPlugins[i].Target]; exists {
cmd := cli.GetUnmappedCmdForPlugin(&installedPlugins[i])
for i := range plugins {
if targetCmd, exists := mapTargetToCmd[plugins[i].Target]; exists {
cmd := cli.GetUnmappedCmdForPlugin(&plugins[i])
if cmd != nil {
targetCmd.AddCommand(cmd)
}
}
}

// Hide the targets that currently have no plugins installed.
for _, targetCmd := range mapTargetToCmd {
if len(targetCmd.Commands()) == 0 {
targetCmd.Hidden = true
}
}
return nil
}

Expand Down
137 changes: 113 additions & 24 deletions pkg/command/root_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ type testCLIEnvironment struct {
envVars []string
}

// helper to set up a clean environment to creat a root CLI command
// helper to set up a clean environment to create a root CLI command
func setupTestCLIEnvironment(t *testing.T) testCLIEnvironment {
dir, err := os.MkdirTemp("", "tanzu-cli-root-cmd")
assert.Nil(t, err)
Expand Down Expand Up @@ -752,8 +752,6 @@ func TestTargetCommands(t *testing.T) {
os.Stdout = w
os.Stderr = w

assert.Nil(err)

for _, target := range spec.installedPluginTargets {
pluginName := "dummy"
pi := &cli.PluginInfo{
Expand Down Expand Up @@ -825,6 +823,8 @@ func TestEnvVarsSet(t *testing.T) {
assert.Nil(err)
// Make sure the variable is now set during the call to the CLI
assert.Equal(envVarValue, os.Getenv(envVarName))

os.Unsetenv(envVarName)
}

func setupFakePlugin(dir, pluginName, _ string, commandGroup plugin.CmdGroup, completionType uint8, target configtypes.Target, postInstallResult uint8, hidden bool, aliases []string) error {
Expand Down Expand Up @@ -963,7 +963,7 @@ func TestPluginLevelRemapping(t *testing.T) {
{
test: "one unmapped k8s plugin",
pluginVariants: []fakePluginRemapAttributes{
fakePluginRemapAttributes{
{
name: "dummy",
target: configtypes.TargetK8s,
},
Expand All @@ -974,7 +974,7 @@ func TestPluginLevelRemapping(t *testing.T) {
{
test: "one mapped k8s plugin",
pluginVariants: []fakePluginRemapAttributes{
fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetK8s,
invokedAs: []string{"dummy"},
Expand All @@ -986,7 +986,7 @@ func TestPluginLevelRemapping(t *testing.T) {
{
test: "once mapped, command using original plugin name is not accessible",
pluginVariants: []fakePluginRemapAttributes{
fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetK8s,
invokedAs: []string{"dummy"},
Expand All @@ -998,12 +998,12 @@ func TestPluginLevelRemapping(t *testing.T) {
{
test: "two plugins share aliases shows duplicate warning",
pluginVariants: []fakePluginRemapAttributes{
fakePluginRemapAttributes{
{
name: "dummy",
target: configtypes.TargetK8s,
aliases: []string{"dum"},
},
fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetK8s,
invokedAs: []string{"dummy3"},
Expand All @@ -1016,12 +1016,12 @@ func TestPluginLevelRemapping(t *testing.T) {
{
test: "two plugins sharing aliases does not show duplicate warning if one replaces another",
pluginVariants: []fakePluginRemapAttributes{
fakePluginRemapAttributes{
{
name: "dummy",
target: configtypes.TargetK8s,
aliases: []string{"dum"},
},
fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetK8s,
invokedAs: []string{"dummy"},
Expand All @@ -1030,12 +1030,12 @@ func TestPluginLevelRemapping(t *testing.T) {
},
args: []string{"dummy", "say", "hello"},
expected: []string{"hello"},
unexpected: []string{"duplciated across plugins"},
unexpected: []string{"duplicated across plugins"},
},
{
test: "map to deeper subcommand is valid as long as parent exists",
pluginVariants: []fakePluginRemapAttributes{
fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetK8s,
invokedAs: []string{"dummy", "kubernetes dummy"},
Expand All @@ -1044,10 +1044,34 @@ func TestPluginLevelRemapping(t *testing.T) {
args: []string{"kubernetes", "dummy", "say", "hello"},
expected: []string{"hello"},
},
{
test: "k8s-target plugins also maps to kubernetes command group",
pluginVariants: []fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetK8s,
invokedAs: []string{"dummy"},
},
},
args: []string{"kubernetes", "dummy", "say", "hello"},
expected: []string{"hello"},
},
{
test: "k8s-target plugins remapping in kubernetes command group also remap top-level command group",
pluginVariants: []fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetK8s,
invokedAs: []string{"kubernetes dummy"},
},
},
args: []string{"dummy", "say", "hello"},
expected: []string{"hello"},
},
{
test: "map to deeper subcommand is invalid if parent missing",
pluginVariants: []fakePluginRemapAttributes{
fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetK8s,
invokedAs: []string{"dummy", "nonexistentprefix dummy"},
Expand All @@ -1059,7 +1083,7 @@ func TestPluginLevelRemapping(t *testing.T) {
{
test: "one mapped tmc plugin",
pluginVariants: []fakePluginRemapAttributes{
fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetTMC,
invokedAs: []string{"dummy", "mission-control dummy"},
Expand All @@ -1071,13 +1095,13 @@ func TestPluginLevelRemapping(t *testing.T) {
{
test: "two plugins remapped to same location will warn",
pluginVariants: []fakePluginRemapAttributes{
fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetK8s,
invokedAs: []string{"dummy"},
aliases: []string{"dum"},
},
fakePluginRemapAttributes{
{
name: "dummy3",
target: configtypes.TargetK8s,
invokedAs: []string{"dummy"},
Expand All @@ -1087,15 +1111,82 @@ func TestPluginLevelRemapping(t *testing.T) {
args: []string{"dummy", "say", "hello"},
expected: []string{"hello", "Warning, multiple command groups are being remapped to the same command names : \"dummy, dummy\""},
},
{
test: "mapped plugin with supportedContextType, command is hidden at target when no active context",
pluginVariants: []fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetK8s,
invokedAs: []string{"dummy"},
aliases: []string{"dum"},
supportedContextType: []configtypes.ContextType{configtypes.ContextTypeTanzu},
},
},
args: []string{"kubernetes"},
expected: []string{"No plugins are currently installed for the \"kubernetes\" target"},
},
{
test: "mapped plugin is only one for target, top level should show target",
pluginVariants: []fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetK8s,
invokedAs: []string{"dummy"},
aliases: []string{"dum"},
},
},
args: []string{""},
expected: []string{"kubernetes"},
},
{
test: "mapped plugin is only one for target with no active context for type hides target",
pluginVariants: []fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetTMC,
invokedAs: []string{"dummy"},
aliases: []string{"dum"},
},
},
args: []string{"mission-control"},
expected: []string{"No plugins are currently installed for the \"mission-control\" target"},
},
{
test: "with supportedContextType and no invokedAs, command is hidden at target when no active context",
pluginVariants: []fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetTMC,
supportedContextType: []configtypes.ContextType{configtypes.ContextTypeTMC},
aliases: []string{"dum"},
},
},
args: []string{"mission-control"},
expected: []string{"No plugins are currently installed for the \"mission-control\" target"},
},
{
test: "with supportedContextType set, target not shown at top level when no active context",
pluginVariants: []fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetTMC,
supportedContextType: []configtypes.ContextType{configtypes.ContextTypeTMC},
aliases: []string{"dum"},
},
},
activeContextType: configtypes.ContextTypeTanzu,
args: []string{},
unexpected: []string{"mission-control"},
},
{
test: "mapping plugins when conditional on active contexts",
pluginVariants: []fakePluginRemapAttributes{
fakePluginRemapAttributes{
{
name: "dummy",
target: configtypes.TargetK8s,
aliases: []string{"dum"},
},
fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetK8s,
invokedAs: []string{"dummy"},
Expand All @@ -1111,12 +1202,12 @@ func TestPluginLevelRemapping(t *testing.T) {
{
test: "no mapping if active context not one of supportContextType list",
pluginVariants: []fakePluginRemapAttributes{
fakePluginRemapAttributes{
{
name: "dummy",
target: configtypes.TargetK8s,
aliases: []string{"dum"},
},
fakePluginRemapAttributes{
{
name: "dummy2",
target: configtypes.TargetK8s,
invokedAs: []string{"dummy"},
Expand All @@ -1132,12 +1223,12 @@ func TestPluginLevelRemapping(t *testing.T) {
{
test: "nesting plugin within another plugin is not supported",
pluginVariants: []fakePluginRemapAttributes{
fakePluginRemapAttributes{
{
name: "dummy",
target: configtypes.TargetK8s,
aliases: []string{"dum"},
},
fakePluginRemapAttributes{
{
name: "deeper",
target: configtypes.TargetK8s,
invokedAs: []string{"kubernetes dummy deeper"},
Expand Down Expand Up @@ -1172,8 +1263,6 @@ func TestPluginLevelRemapping(t *testing.T) {
os.Stdout = w
os.Stderr = w

assert.Nil(err)

if spec.activeContextType != "" {
err = config.SetContext(&configtypes.Context{Name: "test-context", ContextType: spec.activeContextType}, true)
assert.Nil(err)
Expand Down

0 comments on commit 2781c8f

Please sign in to comment.