-
Notifications
You must be signed in to change notification settings - Fork 64
✨ Migrate operator-controller cli handling to cobra #1717
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
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Some notes:
|
@grokspawn could I get the workflow approved? I'd like to know id there's anything I need to fix. Thank you |
This needs a rebase. Also, you'll likely want to do catalogd/cmd/catalogd/main.go as well (if not in this PR, then another). |
@o-farag Sorry about the delay in reviewing the PR. Once you rebase the PR will review it. Thanks. |
I believe this PR takes care of catalogd: #1598 |
a99c9b1
to
c491348
Compare
@LalatenduMohanty thanks for letting me know, I've rebased it |
/retest |
@o-farag: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
The e2e-kind test runs successfully on my own repo, and I have no failing tests locally |
Looks like a flake. We can rerun it to test once the PR review is done. |
Signed-off-by: Omar Farag <[email protected]>
Signed-off-by: Omar Farag <[email protected]>
Signed-off-by: Omar Farag <[email protected]>
c491348
to
d04c967
Compare
setupLog.Error(err, "unable to ensure empty cache directory") | ||
os.Exit(1) | ||
return err |
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.
By replacing os.Exit(1) with return err
it is changing the binary's behavior little bit. With the current change we will error out if flags are not used properly and print the usage as well. Previously we wont print the usage. But the new behavior is better IMO.
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.
/lgtm
/hold for someone from the maintainers group to take a look. Maintainers, feel free to remove the hold. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1717 +/- ##
==========================================
- Coverage 68.29% 68.24% -0.05%
==========================================
Files 63 63
Lines 5116 5121 +5
==========================================
+ Hits 3494 3495 +1
- Misses 1392 1396 +4
Partials 230 230
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
/lgtm
/unhold |
Thank you for your contribution! |
05415ef
Description
Migrate operator-controller cli command handling to cobra
Maintain/update flag handling by migrating to cobra as requested @ #1650
Changes:
Reviewer Checklist