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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Horiodino
Copy link

This PR fixes the issue described in operator-framework/operator-controller#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:

  1. Cobra Root Command:

    • Introduced the rootCmd for top-level command handling, providing a clean and standardized entry point for the CLI.
  2. Flag Migration:

    • Migrated all existing flags from the standard flag package to Cobra’s cmd.Flags() and cmd.PersistentFlags().
  3. Subcommand Restructuring:

    • Refactored and restructured any existing subcommands under Cobra’s subcommand system.
  4. Helper Functions:

    • Refined the main function by extracting logic into dedicated helper functions.

@Horiodino Horiodino requested a review from a team as a code owner January 3, 2025 09:39
@Horiodino Horiodino changed the title Migrate Command Handling to Cobra for Simplified Flag Management (#493) Migrate Command Handling to Cobra for Simplified Flag Management Jan 3, 2025
@Horiodino Horiodino changed the title Migrate Command Handling to Cobra for Simplified Flag Management ✨ Migrate Command Handling to Cobra for Simplified Flag Management Jan 3, 2025
@joelanford
Copy link
Member

Hi @Horiodino! Thanks for contributing this! It looks pretty good as a first pass, but before I get too far into reviewing this, I wanted to let you know that we just merged a PR in operator-controller that moves all of catalogd over there to make it a monorepo. And we're planning to close out/transfer issues/PRs here and then archive this repo.

The catalogd main.go is here now: https://github.com/operator-framework/operator-controller/blob/main/catalogd/cmd/catalogd/main.go

We have a few options, I think:

  1. You can re-open this PR against operator-controller and continue the review there. I think this would be easiest since I don't think we actually changed cmd/catalogd/main.go when we copied it over.
  2. We can do a review here, get it spruced up, and then figure out how to get it back into operator-controller. But this has the risk that catalogd/main.go in the operator-controller repo may change in the meantime, and that could cause some problems.
  3. You can totally say "This was best effort, and I'm not interested in following through" and that will be just fine since we pulled the rug out suddenly. Maintainers would likely eventually get to this and do it.

Copy link

codecov bot commented Jan 9, 2025

Codecov Report

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

Project coverage is 36.79%. Comparing base (1dbab8e) to head (8f27a9a).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
cmd/manager/main.go 0.00% 185 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #500      +/-   ##
==========================================
- Coverage   37.20%   36.79%   -0.41%     
==========================================
  Files          15       15              
  Lines        1258     1272      +14     
==========================================
  Hits          468      468              
- Misses        740      754      +14     
  Partials       50       50              

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

@Horiodino
Copy link
Author

Hey @joelanford, no problem at all! I'll go with the first point and open a new PR in the operator-controller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants