-
Notifications
You must be signed in to change notification settings - Fork 6.8k
feat(CLI): Allow unsetting annotations on cluster #26177
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
base: master
Are you sure you want to change the base?
Conversation
✅ Preview Environment deployed on Bunnyshell
See: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
58eeb1b to
0f65c09
Compare
This allows users to unset cluster annotations by using a special syntax that is equivalent of "kubectl annotate". The same functionality is currently available through the UI. Example usage: ``` $ argocd cluster set mycluster --annotation unwanted=12345 Cluster 'mycluster' updated. $ argocd cluster get mycluster | yq .annotations argocd.argoproj.io/refresh: "2025-08-04T14:39:55Z" unwanted: '12345-0123' $ $ argocd cluster set mycluster --annotation unwanted- Cluster 'mycluster' updated. $ ./dist/argocd cluster get mycluster | yq .annotations argocd.argoproj.io/refresh: "2025-08-04T14:39:55Z" $ ``` Signed-off-by: Marek Skrobacki <marek.skrobacki@rackspace.co.uk>
0f65c09 to
6cf2e45
Compare
pjiang-dev
left a comment
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 for the PR. Added a few comments.
Also one potential concern is if certain annotations are used for security policies (e.g., access controls), allowing deletion could bypass those controls.
| const labelFieldDelimiter = "=" | ||
| const ( | ||
| labelFieldDelimiter = "=" | ||
| deletionMarker = "\x00DELETE\x00" |
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 deletion marker seems a bit hacky/awkward. Consider using a new type with Delete option.
type LabelUpdate struct {
Value string
Delete bool
}
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.
I like that idea - will look into that. thanks for the feedback
| fields := strings.Split(r, labelFieldDelimiter) | ||
| if len(fields) != 2 { | ||
| return nil, fmt.Errorf("labels should have key%svalue, but instead got: %s", labelFieldDelimiter, r) | ||
| if strings.HasSuffix(r, "-") { |
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.
What happens if an annotation legitimately has the '-' suffix ?
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.
Not possible: https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/#syntax-and-character-set
The name segment is required and must be 63 characters or less, beginning and ending with an alphanumeric character ([a-z0-9A-Z]) with dashes (-), underscores (_), dots (.), and alphanumerics between.
This allows users to unset cluster annotations by using a special syntax that is equivalent of "kubectl annotate".
The same functionality is currently available through the UI.
Example usage:
Checklist:
and UIto expose my feature, or I plan to submit a second PR with them.