-
Notifications
You must be signed in to change notification settings - Fork 979
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
Add template labels support to kubernetes labels #2473
base: main
Are you sure you want to change the base?
Add template labels support to kubernetes labels #2473
Conversation
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.
Had a quick skim over this, seems OK so far.
One thing caught my eye - see my comment bellow.
I'll have a closer look and try it out when I get some spare cycles next week.
We'd benefit from a quick glance from @jrhouston too. He's more familiar with this part of the code than I am.
metadata = mmm | ||
} | ||
if l, ok := metadata["f:labels"].(map[string]interface{}); ok { | ||
labels = l |
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 labels
is declared outside the for loop, this will overwrite it every time there is a match on manager
. Should this rather be an append-type operation, adding the contents of l
to labels
?
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
labels
is declared outside the for loop, this will overwrite it every time there is a match onmanager
. Should this rather be an append-type operation, adding the contents ofl
tolabels
?
I have directly copied from template_annotations for this part, I will check for this.
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
labels
is declared outside the for loop, this will overwrite it every time there is a match onmanager
. Should this rather be an append-type operation, adding the contents ofl
tolabels
?
Hi @alexsomesan yeah, I agree there could be potential issues in theory, however, I wasn't able to reproduce it with kubectl
.
I tried kubectl apply
and kubectl patch
on same object multiple times, however, looks like the server will store different manager kubectl-client-side-apply
and kubectl-patch
.
Do you have any suggestions on reproducing this issue? Or you guys are fine for me to change the code (without testing this case).
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.
Hi @theloneexplorerquest, this issue is specific to using server-side-apply
to access the API, however it appears you may be using client-side-apply
as per the name of manager. kubectl
by default uses client-side-apply
and requires you to explicitly state --server-side
in order to use the server-side-apply
. Here's a useful link.
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.
Hi @theloneexplorerquest, this issue is specific to using
server-side-apply
to access the API, however it appears you may be usingclient-side-apply
as per the name of manager.kubectl
by default usesclient-side-apply
and requires you to explicitly state--server-side
in order to use theserver-side-apply
. Here's a useful link.
Hi @sheneska thanks for reply. Yeah, I actually tried --server-side
, however, I wasn't able to generate manifest with two field managers both named kubectl
, do you have suggestion for that?
Description
resolve #2310
Acceptance tests
Output from acceptance testing:
Release Note
Release note for CHANGELOG:
References
Community Note