-
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
Conversation
ankur22
left a comment
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.
Generally looks good to me, but i'm not an expert in this particular area of k6.
One thing that I think might be worth doing is adding tests for the failure cases, e.g. when duplicate sub command extensions are added.
@ankur22, Thank you, you are absolutely right. I added a few tests to the |
oleiade
left a comment
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.
Left a couple of non-blocking comments behind. This looks pretty good, and I'm excited to try it out 👏🏻
ext/ext.go
Outdated
| mx.RLock() | ||
| defer mx.RUnlock() | ||
|
|
||
| js, out := extensions[JSExtension], extensions[OutputExtension] |
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.
Would it make sense to also add the Subcommand extensions here? That way users using a subcommand extension can see it listed by the version command when extensions are included?
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.
@oleiade you are absolutely right. I will add.
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.
@oleiade, done, could you approve again please
I don't think we are supporting extensions ONLY for the cloud. My understanding is that we still want to have a thriving OSS extensions ecosystem.
My experience as an extension developer is that having to distribute another binary to supplement an extension is not the best user experience. In my case, it was to help people check the configuration for the disruptor. something like |
Agree here. I started looking at this issue for output extensions. As I understand it, we have the output in the consolidated config, including those defined in the CLI flags. We also have the outputs for extension in the catalog, so we could map the output name to the extension in the catalog. Maybe we could do something similar with subcommands: parse the CLI and collect the name of the subcommand and make it available in the consolidated config. |
mstoykov
left a comment
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.
I have few nit comments on stuff that just do not make sense in the code IMO.
Apart from that it LGTM. I do like the k6 x something more than the k6 x-something so 👍 .
I am still not convinced of the usefulness of this. Most other applications that use other binaries sometimes have ways to ... keep them updated or install them, but usually do not make them to be part of the binary and/or to be dependant on the project. Think mason.nvim . But again in those cases it is a way to have bianries you need to have your neovim configu work - like installing gopls, dlv and co. And keep them updated.
git I think does something similar where git something looks for git-something in soem cases - can not find citation on this though ...
All in all, this seems to be creating the case that we will be introducing k6 specific solutions that can be generic, but now have to be tied up to k6 - k6 httpbin.
k6 new and the distruptor have some merit, but I dobut we will move k6 new out and I feel like the distruptor has a lot more complexity and problems in other areas that ... haivng another binary is not that big of an issue.
Again this comparing maintaince and development burdance to the usefulness - if you say that "oh this is just 313 lines of code" thatn .. yeah it likely is useful. If you think abotu this making us now requiring to maintain all the APIs exposed by this - then ... I would argue that this feature needs to at least be making us coffee in the morning :)
Irregardless as mentioned before I take all APIs to be only around until next version and hten we will see 🤷
internal/cmd/root.go
Outdated
| // Add extension subcommands | ||
| for sc := range extensionSubcommands(gs, xCmd.Commands()) { | ||
| xCmd.AddCommand(sc) | ||
| } |
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.
nit: IMO this should be in getX
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.
I agree, due to the refactoring under x, it should be in getX(), I will change it.
|
I think the correct model to compare this new functionality to is kubectl plugins. |
Description
This PR adds support for subcommand extensions, allowing external modules to register custom subcommands under the
k6 xnamespace.Changes
Core Extension Framework
SubcommandExtensiontype toext/ext.goSubcommand Package (
subcommand/extension.go)subcommandpackage for registering subcommand extensionsConstructorfunction type that creates cobra.Command instancesRegisterExtensionfunction for registering subcommands during initGlobalStatefor access to k6 runtime stateIntegration with k6 CLI (
internal/cmd/)New
k6 xCommand NamespacegetXfunction ininternal/cmd/subcommand.goxparent command for all extension subcommandsExtension Loading
Added
extensionSubcommandsiterator ininternal/cmd/subcommand.goAdded
getCmdForExtensionhelper with strict validationsubcommand.ConstructorIntegrated into root command (
internal/cmd/root.go)k6 xnamespaceTesting
Comprehensive Test Coverage (
internal/cmd/subcommand_test.go)TestExtensionSubcommands- 5 test cases covering:TestXCommandHelpDisplayCommands- Verifies help outputk6 x helpTest infrastructure (
internal/cmd/root_test.go)registerTestSubcommandExtensionshelper with sync.Oncexcommand presenceImportant Notes
k6 xUse Case
Extensions register subcommands during init:
Users invoke:
Docs grafana/k6-docs#2143
Closes #5398