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

cilium: export intermediate cobra.Commands #26265

Merged
merged 1 commit into from Jul 6, 2023

Conversation

lmb
Copy link
Contributor

@lmb lmb commented Jun 15, 2023

Export all cobra.Command which do not have a Run function set. This allows extending the CLI behaviour from outside the package without allowing modifications to individual commands.

@lmb lmb added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/modularization labels Jun 15, 2023
@lmb lmb self-assigned this Jun 15, 2023
@lmb lmb marked this pull request as ready for review June 19, 2023 09:24
@lmb lmb requested review from a team as code owners June 19, 2023 09:24
@lmb lmb requested review from jibi and derailed June 19, 2023 09:24
@lmb
Copy link
Contributor Author

lmb commented Jun 19, 2023

/test

@lmb lmb added the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Jun 19, 2023
Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@lmb Tx for this PR!
Could be totally out of context here but not sure if I am missing the intent here...
It would be great to highlight some of the use cases this pr would enable to further understand the motivation?
Especially the part about without allowing modifications to individual commands?
Isn't it the case that if we make these package globals then one can override the entire command structure since cobra exports all the interesting bits?
Especially in light that rootCmd would now be exported.

Am I wrong about that?

@lmb
Copy link
Contributor Author

lmb commented Jun 26, 2023

It would be great to highlight some of the use cases this pr would enable to further understand the motivation?

The use case is to build a binary which exposes a superset cilium commands. Imagine I want to build a sillyum binary that has the cowsay subcommand or something similar. Exporting the Root command (and others) allows doing that without having to patch the source code.

Isn't it the case that if we make these package globals then one can override the entire command structure since cobra exports all the interesting bits?

You can't change arbitrary subcommands from what I can tell (since those are unexported), but other bits are up for grabs, yes!

I have two ideas if you think this level of openness is problematic:

  1. Move the exported commands into an internal subpackage. This means that only true forks of the repo can access the root command. Importing it in a different Go module is prohibited.
  2. Instead of exporting the commands, export functions like RootCmd() MyCommandType where MyCommandType is something like the following:
type MyCommandType interface{
  AddCommand(...)
 // Prevent others from implementing the interface. May not be needed.
  unexported()
}

@lmb
Copy link
Contributor Author

lmb commented Jun 28, 2023

@derailed thoughts?

@lmb
Copy link
Contributor Author

lmb commented Jul 4, 2023

After discussing on Slack, it turns out that exporting the root command is equivalent to exporting all cobra commands anyways, due to https://pkg.go.dev/github.com/spf13/cobra#Command.Commands So I think we're not making anything worse by exporting intermediate commands here.

Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@lmb LGTM

@lmb lmb removed the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Jul 5, 2023
@lmb
Copy link
Contributor Author

lmb commented Jul 5, 2023

/test

Export all cobra.Command which do not have a Run function set.
This allows extending the CLI behaviour from outside the package
without allowing modifications to individual commands.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
@lmb
Copy link
Contributor Author

lmb commented Jul 5, 2023

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 5, 2023
@borkmann borkmann merged commit 447c911 into cilium:main Jul 6, 2023
64 of 65 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/modularization ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants