-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add subcommand extension support #5399
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
base: master
Are you sure you want to change the base?
Changes from 5 commits
964f2f7
4f21256
a6e43a3
c53a720
d178d7b
b8bc100
6d85c9a
afdba84
05bc35d
3a8eb7a
fa94a46
fa4a7d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| package cmd | ||
|
|
||
| import ( | ||
| "iter" | ||
|
|
||
| "github.com/sirupsen/logrus" | ||
| "github.com/spf13/cobra" | ||
| "go.k6.io/k6/cmd/state" | ||
| "go.k6.io/k6/ext" | ||
| "go.k6.io/k6/subcommand" | ||
| ) | ||
|
|
||
| // extensionSubcommands returns an iterator over all registered subcommand extensions | ||
| // that are not already defined in the given slice of commands. | ||
| func extensionSubcommands(gs *state.GlobalState, defined []*cobra.Command) iter.Seq[*cobra.Command] { | ||
| already := make(map[string]struct{}, len(defined)) | ||
| for _, cmd := range defined { | ||
| already[cmd.Name()] = struct{}{} | ||
| } | ||
|
|
||
| return func(yield func(*cobra.Command) bool) { | ||
| for _, extension := range ext.Get(ext.SubcommandExtension) { | ||
| if _, exists := already[extension.Name]; exists { | ||
| gs.Logger.WithFields(logrus.Fields{"name": extension.Name, "path": extension.Path}). | ||
| Warnf("subcommand already exists") | ||
| continue | ||
| } | ||
|
|
||
| already[extension.Name] = struct{}{} | ||
|
|
||
| if !yield(getCmdForExtension(extension, gs)) { | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // getCmdForExtension gets a *cobra.Command for the given subcommand extension. | ||
| func getCmdForExtension(extension *ext.Extension, gs *state.GlobalState) *cobra.Command { | ||
| ctor, ok := extension.Module.(subcommand.Constructor) | ||
| if !ok { | ||
| gs.Logger.WithFields(logrus.Fields{"name": extension.Name, "path": extension.Path}). | ||
| Fatalf("subcommand's constructor does not implement the subcommand.Constructor") | ||
| } | ||
|
|
||
| cmd := ctor(gs) | ||
|
|
||
| // Validate that the command's name matches the extension name. | ||
| if cmd.Name() != extension.Name { | ||
| gs.Logger.WithFields(logrus.Fields{"name": extension.Name, "path": extension.Path}). | ||
| Fatalf("subcommand's command name (%s) does not match the extension name (%s)", cmd.Name(), extension.Name) | ||
| } | ||
|
Comment on lines
60
to
63
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not a fan of using Fatal as a way to return errors and ending execution. It would be a lot nicer to just use errors.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't really like the use of Fatal log either. Upon closer inspection, this is not a runtime error, but a faulty extension implementation. In this case, I think the use of panic is justified, I changed it to panic. |
||
|
|
||
| return cmd | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| package cmd | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/spf13/cobra" | ||
| "github.com/stretchr/testify/require" | ||
| "go.k6.io/k6/internal/cmd/tests" | ||
| ) | ||
|
|
||
| func TestExtensionSubcommands(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| registerTestSubcommandExtensions(t) | ||
|
|
||
| t.Run("returns all extension subcommands", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| ts := tests.NewGlobalTestState(t) | ||
| defined := []*cobra.Command{} | ||
|
|
||
| var commands []*cobra.Command | ||
| for cmd := range extensionSubcommands(ts.GlobalState, defined) { | ||
| commands = append(commands, cmd) | ||
| } | ||
|
|
||
| // Should have at least the 3 test extensions we registered | ||
| require.GreaterOrEqual(t, len(commands), 2) | ||
|
|
||
| // Check that our test commands are present | ||
| commandNames := make(map[string]bool) | ||
| for _, cmd := range commands { | ||
| commandNames[cmd.Name()] = true | ||
| } | ||
|
|
||
| require.True(t, commandNames["test-cmd-1"], "test-cmd-1 should be present") | ||
| require.True(t, commandNames["test-cmd-2"], "test-cmd-2 should be present") | ||
| require.True(t, commandNames["test-cmd-3"], "test-cmd-3 should be present") | ||
| }) | ||
|
|
||
| t.Run("filters out already defined commands", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| ts := tests.NewGlobalTestState(t) | ||
|
|
||
| // Create a command with the same name as one of our extensions | ||
| defined := []*cobra.Command{ | ||
| { | ||
| Use: "test-cmd-1", | ||
| Short: "Already defined command", | ||
| Run: func(_ *cobra.Command, _ []string) {}, | ||
| }, | ||
| } | ||
|
|
||
| var commands []*cobra.Command | ||
| for cmd := range extensionSubcommands(ts.GlobalState, defined) { | ||
| commands = append(commands, cmd) | ||
| } | ||
|
|
||
| // Check that test-cmd-1 is NOT in the results | ||
| for _, cmd := range commands { | ||
| require.NotEqual(t, "test-cmd-1", cmd.Name(), "test-cmd-1 should be filtered out") | ||
| } | ||
|
|
||
| // But test-cmd-2 and test-cmd-3 should still be present | ||
| commandNames := make(map[string]bool) | ||
| for _, cmd := range commands { | ||
| commandNames[cmd.Name()] = true | ||
| } | ||
|
|
||
| require.True(t, commandNames["test-cmd-2"], "test-cmd-2 should be present") | ||
| require.True(t, commandNames["test-cmd-3"], "test-cmd-3 should be present") | ||
| }) | ||
|
|
||
| t.Run("prevents duplicate extensions", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| ts := tests.NewGlobalTestState(t) | ||
| defined := []*cobra.Command{} | ||
|
|
||
| // Collect all commands | ||
| var commands []*cobra.Command | ||
| for cmd := range extensionSubcommands(ts.GlobalState, defined) { | ||
| commands = append(commands, cmd) | ||
| } | ||
|
|
||
| // Check for duplicates | ||
| seen := make(map[string]bool) | ||
| for _, cmd := range commands { | ||
| require.False(t, seen[cmd.Name()], "command %s should not appear twice", cmd.Name()) | ||
| seen[cmd.Name()] = true | ||
| } | ||
| }) | ||
|
|
||
| t.Run("returns commands with correct properties", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| ts := tests.NewGlobalTestState(t) | ||
| defined := []*cobra.Command{} | ||
|
|
||
| for cmd := range extensionSubcommands(ts.GlobalState, defined) { | ||
| require.NotEmpty(t, cmd.Use, "command should have a Use field") | ||
|
|
||
| switch cmd.Use { | ||
| case "test-cmd-1": | ||
| require.Equal(t, "Test command 1", cmd.Short) | ||
| case "test-cmd-2": | ||
| require.Equal(t, "Test command 2", cmd.Short) | ||
| case "test-cmd-3": | ||
| require.Equal(t, "Test command 3", cmd.Short) | ||
| } | ||
| } | ||
| }) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| // Package subcommand provides functionality for registering k6 subcommand extensions. | ||
| // | ||
| // This package allows external modules to register new subcommands that will be | ||
| // available in the k6 CLI. Subcommand extensions are registered during | ||
| // package initialization and are called when the corresponding subcommand is invoked. | ||
| package subcommand | ||
|
|
||
| import ( | ||
| "github.com/spf13/cobra" | ||
| "go.k6.io/k6/cmd/state" | ||
| "go.k6.io/k6/ext" | ||
| ) | ||
|
|
||
| // Constructor is a function type that creates a new cobra.Command for a subcommand extension. | ||
| // It receives a GlobalState instance that provides access to configuration, logging, | ||
| // file system, and other shared k6 runtime state. The returned Command will be | ||
| // integrated into k6's CLI as a subcommand. | ||
| type Constructor func(*state.GlobalState) *cobra.Command | ||
|
|
||
| // RegisterExtension registers a subcommand extension with the given name and constructor function. | ||
| // | ||
| // The name parameter specifies the subcommand name that users will invoke (e.g., "k6 <name>"). | ||
| // The constructor function will be called when k6 initializes to create the cobra.Command | ||
| // instance for this subcommand. | ||
| // | ||
| // This function must be called during package initialization (typically in an init() function) | ||
| // and will panic if a subcommand with the same name is already registered. | ||
| // | ||
|
Comment on lines
14
to
32
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer that we have a warning here that modifying the GlobalState is not intended and likely will lead to not working k6. I do not really like that we provide basically full access to extensions, and this doesn't make this any better. As usual most k6 APIs are meant for internal usage, and their usage for extensions was experimental and we are now continuing without actually making this different. This is slightly a mute poitn as I do expect k6 v2 will break eveyr extension whether on purpose or by accident, so 🤷
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, a warning would be useful that it is not the intention to modify the GlobalState. I will add a sentence about this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a warning: extension.go:19-21 |
||
| // The name parameter and the returned Command's Name() must match. | ||
| func RegisterExtension(name string, c Constructor) { | ||
| ext.Register(name, ext.SubcommandExtension, c) | ||
| } | ||
|
Comment on lines
+34
to
+36
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do think that it is a requirement that we do not let people just have any names, but similar to js extension have a prefix that they need to have. As otherwise any new command in core will break an extension that uses that same command name.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are absolutely right. I missed the namespace for output extensions before. I also thought about the namespace for subcommand extensions, but I haven't found a good solution until now. That is, the |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be used only in tests and should then be put in the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it is used to add registered subcommands to the root command: root.go:90-92