-
Notifications
You must be signed in to change notification settings - Fork 59
Added labels and annotations flags to cluster and policy create #565
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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #565 +/- ##
==========================================
- Coverage 59.71% 59.67% -0.05%
==========================================
Files 56 56
Lines 5198 5217 +19
==========================================
+ Hits 3104 3113 +9
- Misses 1806 1814 +8
- Partials 288 290 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bigkevmcd
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.
Looks good apart from the parsing part which I think we should fix.
|
|
||
| func parseKeyValuePairs(pairs []string, pairType string) map[string]string { | ||
| resultMap := make(map[string]string) | ||
|
|
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 could maybe be a bit more robust as it'll be receiving user data from the CLI?
Maybe if len(keyValue) != 2 then it's a problem, and don't limit the split?
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.
@bigkevmcd for the labels that would work, but annotations permits the value to contains = (or other chars).
Should I split the parsing logic in two? Not sure if there is a Kubernetes func that can be used to validate that.
Also an empty label, or empty annotation is still valid, so len could be 1.
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.
@enrichman Good questions...I'm not sure...it feels like work for something that can be fixed with kubectl label guess :-)
I just tested this:
$ kubectl label secret/my-test-secret example.com/test=`
secret/my-test-secret labeled
Then...
$ kubectl get secret/my-test-secret -o json | jq ".metadata.labels"
{
"example.com/test": ""
}
So...I guess that behaviour is desirable 😄
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.
Yep, I've just checked the doc and it seems to be documented as well:
Valid label value:
- must be 63 characters or less (can be empty),
- unless empty, must begin and end with an alphanumeric character ([a-z0-9A-Z]),
- could contain dashes (-), underscores (_), dots (.), and alphanumerics between.
Thank you!
Fix #355
There is sometimes the need to label and annotate the k3k resources.
This PR adds the
--labelsand--annotationsflags to thecreate clusterandcreate policycommands.