✨ Migrate Command Handling to Cobra for Simplified Flag Management #1598

Horiodino · 2025-01-10T16:28:55Z

This PR fixes the issue described in #1567.

Description:

This PR migrates the command-line handling logic in the cmd/manager/main.go file 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:
- Introduced the rootCmd for top-level command handling, providing a clean and standardized entry point for the CLI.
Flag Migration:
- Migrated all existing flags from the standard flag package to Cobra’s cmd.Flags() and cmd.PersistentFlags().
Subcommand Restructuring:
- Refactored and restructured any existing subcommands under Cobra’s subcommand system.
Helper Functions:
- Refined the main function by extracting logic into dedicated helper functions.

netlify · 2025-01-10T16:29:13Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`fc34541`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/67a253bdf754a20008c2d695
😎 Deploy Preview	https://deploy-preview-1598--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

kevinrizza · 2025-01-10T19:56:09Z

👋 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.

LalatenduMohanty · 2025-01-10T20:06:32Z

/assign @LalatenduMohanty

codecov · 2025-01-10T22:21:22Z

Codecov Report

Attention: Patch coverage is 0% with 196 lines in your changes missing coverage. Please review.

Project coverage is 67.60%. Comparing base (9b08aea) to head (950c722).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
catalogd/cmd/catalogd/main.go	0.00%	196 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1598      +/-   ##
==========================================
- Coverage   67.74%   67.60%   -0.15%     
==========================================
  Files          57       57              
  Lines        4620     4630      +10     
==========================================
  Hits         3130     3130              
- Misses       1265     1275      +10     
  Partials      225      225

Flag	Coverage Δ
e2e	`53.44% <ø> (ø)`
unit	`54.38% <0.00%> (-0.12%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

grokspawn · 2025-01-10T22:35:14Z

Hey @Horiodino! Thanks for your PR (especially since you noticed catalogd's new home and reproduced it here).
Could you do a make verify in your local and commit the dependency updates please?

joelanford · 2025-01-13T22:28:58Z

catalogd/cmd/catalogd/main.go

-	if catalogdVersion {
-		fmt.Printf("%#v\n", version.Version())
-		os.Exit(0)
+	cfg := &config{}


Nit: since config is used entirely inside newRootCmd, it seems like we could declare it there instead?

joelanford · 2025-01-13T22:29:51Z

catalogd/cmd/catalogd/main.go

 	}

+	flags := cmd.PersistentFlags()


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.

joelanford · 2025-01-13T22:32:22Z

catalogd/cmd/catalogd/main.go

+	flags.StringVar(&cfg.globalPullSecret, "global-pull-secret", "", "Global pull secret (<namespace>/<name>)")
+
+	cmd.AddCommand(newVersionCmd())
+	klog.InitFlags(nil)


Maybe it is just me, but I prefer explicitly passing flag.CommandLine here instead of relying on the defaulting that happens within InitFlags.

@joelanford Do you mean you would like to have a func init() { for the flags? More info at https://github.com/spf13/cobra/blob/main/site/content/user_guide.md

I would prefer if we can move the flags to init() method. Because it is easier to maintain in long term.

Could someone please give a final review so we can make the necessary changes? Everyone seems to have different opinions.

joelanford · 2025-01-13T22:34:47Z

catalogd/cmd/catalogd/main.go

+		return err
+	}
+
+	if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil {


Might be nice to call ctrl.SetupSignalHandler() from the inlined Run command, and then update this run function to accept a context.Context.

joelanford · 2025-01-13T22:41:21Z

catalogd/cmd/catalogd/main.go

+		RunE: func(cmd *cobra.Command, args []string) error {
+			return run(cfg)
+		},


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)?

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.

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) }

joelanford · 2025-01-14T20:41:09Z

catalogd/cmd/catalogd/main.go

@@ -147,7 +153,7 @@ func newVersionCmd() *cobra.Command {
 	}
 }

-func run(cfg *config) error {
+func run(cfg *config, ctx context.Context) error {


By convention context.Context should always be the first argument in the function signature.

LalatenduMohanty · 2025-01-21T00:01:38Z

catalogd/cmd/catalogd/main.go

+	flags.StringVar(&cfg.globalPullSecret, "global-pull-secret", "", "Global pull secret (<namespace>/<name>)")
+
+	cmd.AddCommand(newVersionCmd())
+	klog.InitFlags(nil)


I would prefer if we can move the flags to init() method. Because it is easier to maintain in long term.

grokspawn · 2025-01-27T03:05:57Z

Thanks @Horiodino! We can make additional adjustments in subsequent PRs, but I'd like to keep momentum in this area. I think this achieves the original goals, so approving it.

Lets merge this and we can improve the code further.

grokspawn · 2025-02-04T16:36:34Z

Hi @Horiodino . Sorry about the delays here, but it looks like this PR needs a rebase. Can you take care of that?

Signed-off-by: Horiodino <[email protected]>

openshift-merge-robot · 2025-02-06T22:53:48Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Horiodino · 2025-02-07T03:23:43Z

Folks, please! I've already rebased five times. If it's not good, I'll close the PR—but please don’t leave it hanging.

Horiodino requested a review from a team as a code owner January 10, 2025 16:28

openshift-ci bot assigned LalatenduMohanty Jan 10, 2025

joelanford reviewed Jan 13, 2025

View reviewed changes

joelanford reviewed Jan 14, 2025

View reviewed changes

Horiodino force-pushed the refactor-main.go branch from 1c158d1 to 623919d Compare January 15, 2025 09:26

LalatenduMohanty previously requested changes Jan 21, 2025

View reviewed changes

grokspawn previously approved these changes Jan 27, 2025

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2025

Horiodino dismissed grokspawn’s stale review via f9746b6 February 1, 2025 10:21

Horiodino force-pushed the refactor-main.go branch from 623919d to f9746b6 Compare February 1, 2025 10:21

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2025

Horiodino added 4 commits February 4, 2025 23:18

fixed Undefined pullCasDir

d0ae86a

Signed-off-by: Horiodino <[email protected]>

use operator-controller/catalogd.

3c0828a

Signed-off-by: Horiodino <[email protected]>

Update go.mod

ab6e292

Signed-off-by: Horiodino <[email protected]>

Addressed changes as mentioned in PR comments

fc34541

Signed-off-by: Horiodino <[email protected]>

Horiodino force-pushed the refactor-main.go branch from 950c722 to fc34541 Compare February 4, 2025 17:51

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Migrate Command Handling to Cobra for Simplified Flag Management #1598

✨ Migrate Command Handling to Cobra for Simplified Flag Management #1598

Horiodino commented Jan 10, 2025

netlify bot commented Jan 10, 2025 •

edited

Loading

kevinrizza commented Jan 10, 2025

LalatenduMohanty commented Jan 10, 2025

codecov bot commented Jan 10, 2025 •

edited

Loading

grokspawn commented Jan 10, 2025

joelanford Jan 13, 2025

joelanford Jan 13, 2025

joelanford Jan 13, 2025

LalatenduMohanty Jan 21, 2025

LalatenduMohanty Jan 21, 2025

Horiodino Jan 25, 2025

joelanford Jan 13, 2025

joelanford Jan 13, 2025

Horiodino Jan 14, 2025

joelanford Jan 14, 2025

joelanford Jan 14, 2025

LalatenduMohanty Jan 21, 2025

grokspawn commented Jan 27, 2025

grokspawn commented Feb 4, 2025

openshift-merge-robot commented Feb 6, 2025

Horiodino commented Feb 7, 2025

✨ Migrate Command Handling to Cobra for Simplified Flag Management #1598

Are you sure you want to change the base?

✨ Migrate Command Handling to Cobra for Simplified Flag Management #1598

Conversation

Horiodino commented Jan 10, 2025

This PR fixes the issue described in #1567.

Description:

Key Changes:

netlify bot commented Jan 10, 2025 • edited Loading

✅ Deploy Preview for olmv1 ready!

kevinrizza commented Jan 10, 2025

LalatenduMohanty commented Jan 10, 2025

codecov bot commented Jan 10, 2025 • edited Loading

Codecov Report

grokspawn commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grokspawn commented Jan 27, 2025

grokspawn commented Feb 4, 2025

openshift-merge-robot commented Feb 6, 2025

Horiodino commented Feb 7, 2025

netlify bot commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading