-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Improves Docker Labels handling and output #3024
Conversation
Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run. |
1 similar comment
Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run. |
Signed the CLA |
@karmi Ops, my bad! Added my email to my profile. Do I need to change anything else? Thanks! |
jenkins, test it |
@rikatz LGTM. Thanks for the contribution. |
@@ -52,24 +51,15 @@ func ExtractContainerName(names []string) string { | |||
return strings.Trim(output, "/") | |||
} | |||
|
|||
func BuildLabelArray(labels map[string]string) []common.MapStr { | |||
func BuildLabelArray(labels map[string]string) common.MapStr { |
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 think the name of this function should change now.
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.
@andrew-moldovan suggestions? SetLabelsFields or BuildLabelsFields is better? Tks
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.
@rikatz I think you meant @andrewkroh :)
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.
Yeah, sorry!
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.
Looking at the what the function does, it seems to de-dot the label names. So how about DeDotLabels()
or DeDotKeys
? I would prefer that all exported functions have godoc accompanying them. So how about adding (assuming that's the name you go with):
// DeDotLabels returns a new common.MapStr containing a copy of the labels
// where the dots in each label name have been changed to an underscore.
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.
OK! Just did a commit right now with the name ConvertContainerLabels, will rename them again to DeDotLabels (as they are directly related to Labels), and also add the comments!
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.
@andrewkroh Did as suggested. Now let's wait the CI tasks complete 😄
Hope this can help!
This PR improves Docker Label handling, by not turning it as an array of JSONs, but instead a set of different fields on a map string.
This is useful specially if you're building dashboards with Kibana and wants to filter based on container labels.
This is one of the items of issue #2629
Hope it helps 😄