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

fixes(#814) allow plugins to extend the 'source' command group #818

Merged
merged 4 commits into from
May 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@
| Check DeleteTimestamp before updating resource
| https://github.com/knative/client/pull/805[#805]

| 🎁
| Allow plugins to extend fixed list of command groups, currently `source`
| https://github.com/knative/client/pull/818[#818]

| 🎁
| Add `-a` flag as an alias for `--annotation`
| https://github.com/knative/client/pull/782[#782]
Expand Down
20 changes: 18 additions & 2 deletions pkg/kn/commands/plugin/list_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@ func (ctx *testContext) createTestPluginWithPath(pluginName string, fileMode os.
}

func TestPluginList(t *testing.T) {

setup := func(t *testing.T) *testContext {
knParams := &commands.KnParams{}
pluginCmd := NewPluginCommand(knParams)
Expand Down Expand Up @@ -159,7 +158,6 @@ func TestPluginList(t *testing.T) {
})

t.Run("with plugins with same name", func(t *testing.T) {

t.Run("warns user about second (in $PATH) plugin shadowing first", func(t *testing.T) {
ctx := setup(t)
defer ctx.cleanup()
Expand Down Expand Up @@ -201,6 +199,24 @@ func TestPluginList(t *testing.T) {
assert.ErrorContains(t, err, "overwrite", "built-in")
assert.Assert(t, util.ContainsAll(ctx.output(), "ERROR", "overwrite", "built-in"))
})

t.Run("allows plugin under the `source` command group", func(t *testing.T) {
ctx := setup(t)
defer ctx.cleanup()

sourceCmd := &cobra.Command{
Use: "source",
}
ctx.rootCmd.AddCommand(sourceCmd)
defer ctx.rootCmd.RemoveCommand(sourceCmd)

err := ctx.createTestPlugin("kn-source-fake", FileModeExecutable, true)
assert.NilError(t, err)

err = ctx.execute("plugin", "list", "--lookup-plugins=true")
assert.NilError(t, err)
Comment on lines +216 to +217
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this also verify the plugin name from the returned list ?

})

})
})
})
Expand Down
17 changes: 17 additions & 0 deletions pkg/kn/commands/plugin/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,20 @@ func BindPluginsFlagToViper(cmd *cobra.Command) {
viper.SetDefault("plugins-dir", commands.Cfg.DefaultPluginDir)
viper.SetDefault("lookup-plugins", false)
}

// AllowedExtensibleCommandGroups the list of command groups that can be
// extended with plugins, e.g., a plugin named `kn-source-kafka` for Kafka
// event sources is allowed. This is defined as a fixed [...]string since
// cannot defined Golang []string constants
var AllowedExtensibleCommandGroups = [...]string{"source"}
Copy link
Collaborator

@navidshaikh navidshaikh May 4, 2020

Choose a reason for hiding this comment

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

I think the command group to extend should be configurable outside the kn, i.e config.
OR
IMO, it shouldn't mind extending command groups if the plugin commands dont clash with kn's own commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure about that since we would not want extensions of core commands

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as per today’s discussion, please merge this PR and I will open an issue with your suggestion with more details and open a 24 hours vote. Thanks @navidshaikh and @rhuss


// InAllowedExtensibleCommandGroups checks that the name is in the list of allowed
// extensible command groups
func InAllowedExtensibleCommandGroups(name string) bool {
for _, groupName := range AllowedExtensibleCommandGroups {
if name == groupName {
return true
}
}
return false
}
10 changes: 10 additions & 0 deletions pkg/kn/commands/plugin/plugin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,13 @@ func TestNewPluginCommand(t *testing.T) {
assert.Assert(t, pluginCmd.Args == nil)
})
}

func TestInAllowedExtensibleCommandGroups(t *testing.T) {
isExtensibleCommand := InAllowedExtensibleCommandGroups("fake")
assert.Assert(t, isExtensibleCommand == false)

for _, name := range AllowedExtensibleCommandGroups {
isExtensibleCommand = InAllowedExtensibleCommandGroups(name)
assert.Assert(t, isExtensibleCommand == true)
}
}
4 changes: 3 additions & 1 deletion pkg/kn/commands/plugin/verifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,9 @@ func (v *pluginVerifier) addErrorIfOverwritingExistingCommand(eaw errorsAndWarni
for _, c := range [][]string{cmds, convertUnderscoresToDashes(cmds)} {
cmd, _, err := v.root.Find(c)
if err == nil {
overwrittenCommands[cmd.CommandPath()] = true
if !InAllowedExtensibleCommandGroups(cmd.Name()) {
overwrittenCommands[cmd.CommandPath()] = true
}
}
}
for command := range overwrittenCommands {
Expand Down
15 changes: 14 additions & 1 deletion pkg/kn/commands/plugin/verifier_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import (
)

func TestPluginVerifier(t *testing.T) {

var (
pluginPath string
rootCmd *cobra.Command
Expand Down Expand Up @@ -143,6 +142,20 @@ func TestPluginVerifier(t *testing.T) {
assert.Assert(t, util.ContainsAll(eaw.errors[0], "overwrite", "kn-plugin"))
})
})

t.Run("when kn plugin in path overwrites 'source' command (which is allowed)", func(t *testing.T) {
setup(t)
defer cleanup(t)
var overwritingPluginPath = CreateTestPlugin(t, "kn-source-test", KnTestPluginScript, FileModeExecutable)
defer DeleteTestPlugin(t, overwritingPluginPath)

t.Run("runs successfully", func(t *testing.T) {
eaw := errorsAndWarnings{}
eaw = verifier.verify(eaw, overwritingPluginPath)
assert.Assert(t, len(eaw.errors) == 0)
assert.Assert(t, len(eaw.warnings) == 0)
})
})
})
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/kn/core/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ func NewDefaultKnCommandWithArgs(rootCmd *cobra.Command,
// only look for suitable extension executables if
// the specified command does not already exist
foundCmd, innerArgs, err := rootCmd.Find(cmdPathPieces)
if err != nil {
if err != nil || plugin.InAllowedExtensibleCommandGroups(foundCmd.Name()) {
err := plugin.HandlePluginCommand(pluginHandler, cmdPathPieces)
if err != nil {
fmt.Fprintf(rootCmd.OutOrStderr(), "Error: unknown command '%s' \nRun 'kn --help' for usage.\n", args[1])
Expand Down