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
feat: allow argocd cluster rotate-auth to accept cluster name #9838
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9838 +/- ##
==========================================
+ Coverage 45.78% 45.82% +0.04%
==========================================
Files 227 227
Lines 26994 27005 +11
==========================================
+ Hits 12359 12375 +16
+ Misses 12952 12941 -11
- Partials 1683 1689 +6
Continue to review full report at Codecov.
|
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.
Nitpicks. :-)
server/cluster/cluster.go
Outdated
if q.Name != "" { | ||
servers, err = s.db.GetClusterServersByName(ctx, q.Name) | ||
if err != nil { | ||
return nil, 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.
Since this is a new potential error, can this be wrapped? status.Errorf("failed to get cluster servers by name: %w", 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.
Added some error context. status.Errorf does not support %w
so I used %v
.
server/cluster/cluster.go
Outdated
} | ||
for _, server := range servers { | ||
if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceClusters, rbacpolicy.ActionUpdate, createRBACObject(clust.Project, server)); err != nil { | ||
return nil, 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.
Even though this wasn't previously wrapped, it probably should be.
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.
Done, let me know if error message can be improved.
server/cluster/cluster.go
Outdated
} | ||
} else { | ||
if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceClusters, rbacpolicy.ActionUpdate, createRBACObject(clust.Project, q.Server)); err != nil { | ||
return nil, 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.
More wrapping, if you've got time.
Signed-off-by: Daniel Helfand <helfand.4@gmail.com>
Signed-off-by: Daniel Helfand <helfand.4@gmail.com>
Signed-off-by: Daniel Helfand <helfand.4@gmail.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!
Closes #9554
Allows the argocd cluster rotate-auth command to accept the cluster name as argument in addition to server.
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: