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
fix: handle repo creation when secret does not have proper label #7617
Conversation
Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
util/db/repository_secrets.go
Outdated
msg := "secret %q doesn't have the proper %q label: please fix the secret or delete it" | ||
return nil, status.Errorf(codes.InvalidArgument, msg, secName, common.LabelKeySecretType) |
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.
This is just a nitpick, but for the compiler (and/or linter) to catch issues with the format string, I think it would be wiser to either have msg := fmt.Sprintf(...)
and use status.Error()
, or just get rid of the intermediate variable msg
and use status.Errorf()
with the format string literal.
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.
The reason why I did that was to avoid having a too long line and avoid scrolling. Too bad the linter is not smart enough to verify the code with variables as well. I picked your first suggestion as it has better code readability. 👍🏻
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.
Thanks @leoluz! LGTM with three minor things (one of them in a separate comment above):
-
Linter is unhappy about import order, please fix
-
While I appreciate embedding the test data (I suppose they don't end up in the binaries we distribute, and only get embedded in the binaries used for testing?), I think this would be a fine separate PR and shouldn't come with this one.
Issue with
A go binary only includes code reachable from its main() entry point. For test binaries main() is the test runner. So unless someone imports the |
Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
Codecov Report
@@ Coverage Diff @@
## master #7617 +/- ##
==========================================
+ Coverage 41.39% 41.44% +0.04%
==========================================
Files 161 161
Lines 21686 21787 +101
==========================================
+ Hits 8978 9030 +52
- Misses 11444 11487 +43
- Partials 1264 1270 +6
Continue to review full report at Codecov.
|
Signed-off-by: Leonardo Luz Almeida <leonardo_almeida@intuit.com>
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
Signed-off-by: Leonardo Luz Almeida leonardo_almeida@intuit.com
Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist: