✨ Migrate Command Handling to Cobra for Simplified Flag Management #1598
Conversation
Signed-off-by: Horiodino <[email protected]>
Signed-off-by: Horiodino <[email protected]>
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
👋 Thanks for the contribution! Quick question, if we're going to do this for the catalogd binary, should we do the same for the operator-controller binary? I.e. https://github.com/operator-framework/operator-controller/blob/main/cmd/operator-controller/main.go I suspect there will be some opinions on the functional refactoring as the controllers are set up (isn't some of that handled by controller runtime and kubebuilder?) and it would probably be good for us to match in those places. I know that's outside the scope of that github issue, but I think that's an artifact of these two binaries being part of separate repositories until very recently. |
|
/assign @LalatenduMohanty |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1598 +/- ##
==========================================
- Coverage 66.68% 66.48% -0.21%
==========================================
Files 57 57
Lines 4584 4598 +14
==========================================
Hits 3057 3057
- Misses 1302 1316 +14
Partials 225 225
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Hey @Horiodino! Thanks for your PR (especially since you noticed catalogd's new home and reproduced it here). |
Signed-off-by: Horiodino <[email protected]>
catalogd/cmd/catalogd/main.go
Outdated
| if catalogdVersion { | ||
| fmt.Printf("%#v\n", version.Version()) | ||
| os.Exit(0) | ||
| cfg := &config{} |
There was a problem hiding this comment.
Nit: since config is used entirely inside newRootCmd, it seems like we could declare it there instead?
catalogd/cmd/catalogd/main.go
Outdated
| } | ||
|
|
||
| flags := cmd.PersistentFlags() |
There was a problem hiding this comment.
Persistent flags apply to all subcommands of this command. Since this command doesn't have any subcommands, I think it would make more sense to use cmd.Flags() here.
| flags.StringVar(&cfg.globalPullSecret, "global-pull-secret", "", "Global pull secret (<namespace>/<name>)") | ||
|
|
||
| cmd.AddCommand(newVersionCmd()) | ||
| klog.InitFlags(nil) |
There was a problem hiding this comment.
Maybe it is just me, but I prefer explicitly passing flag.CommandLine here instead of relying on the defaulting that happens within InitFlags.
catalogd/cmd/catalogd/main.go
Outdated
| return err | ||
| } | ||
|
|
||
| if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { |
There was a problem hiding this comment.
Might be nice to call ctrl.SetupSignalHandler() from the inlined Run command, and then update this run function to accept a context.Context.
catalogd/cmd/catalogd/main.go
Outdated
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| return run(cfg) | ||
| }, |
There was a problem hiding this comment.
I've never been a fan of the RunE abstraction. In almost all cases, there are two steps to a CLI program that need different error handling
- Parse and validate the CLI args, env, etc. If a failure occurs here, report the error and print the usage
- Execute the command. If a failure occurs here, report the error, but DO NOT print the usage.
WDYT about some tweaks to align with that?
I don't recall the exact behavior of RunE, but maybe we'd get reasonable behavior for now if we set cmd.SilenceUsage, so that only the returned errors strings are ever output (but never usage)?
There was a problem hiding this comment.
the run function serves as the main entry point, where all other components are initialized and invoked. Including an error statements.The cobra RunE returns err,ensures proper error handling and clarity.
There was a problem hiding this comment.
My main concern is that I'd like to avoid the usage being printed if the error returned from RunE is unrelated to flag parsing/validation.
For example, if any error occurs after validateTLSConfig, I would not want to see usage output. Perhaps the way to accomplish this is to move the validation code out of run and into a separate validateConfig function. Then RunE could be something along the lines of:
func(cmd *cobra.Command, args []string) error {
if err := validateConfig(cfg); err != nil {
return err
}
cmd.SilenceUsage = true
return run(cfg)
}Signed-off-by: Horiodino <[email protected]>
| @@ -147,7 +153,7 @@ func newVersionCmd() *cobra.Command { | |||
| } | |||
| } | |||
|
|
|||
| func run(cfg *config) error { | |||
| func run(cfg *config, ctx context.Context) error { | |||
There was a problem hiding this comment.
By convention context.Context should always be the first argument in the function signature.
This PR fixes the issue described in #1567.
Description:
This PR migrates the command-line handling logic in the
cmd/manager/main.gofile to utilize the [Cobra CLI framework]. The migration simplifies flag handling, improves readability, and enhances the extensibility of the CLI for future development.Key Changes:
Cobra Root Command:
rootCmdfor top-level command handling, providing a clean and standardized entry point for the CLI.Flag Migration:
flagpackage to Cobra’scmd.Flags()andcmd.PersistentFlags().Subcommand Restructuring:
Helper Functions: