-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(helm): add priorityClassName and extraLabels to kube-agent #12559
Conversation
Signed-off-by: Roman Tkachenko <roman@goteleport.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 with one nit (assuming the tests still pass)
- matchSnapshot: | ||
path: spec.template.spec |
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.
Something I didn't flag on the original PR; not sure why this is changed. It should still be spec.template.spec
AFAIK.
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.
@webvictim Interesting, looks like helm tests actually fail when I rollback this change. Looks like it doesn't render the entire yaml for some reason, only containers section.
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 restored this change because it does seem like it's correct since we want to test against the entire resource spec, not just the template spec.
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 changed it because spec.template.spec
is the spec
of Pods, while deployment annotations stay in the root object. I didn't experience issues with tests even before the change, but I felt this was incorrect, and I fixed it. Sorry for not specifying it.
74adfad
to
9bfa6cd
Compare
* feat(helm): add priorityClassName and extraLabels to kube-agent Signed-off-by: Roman Tkachenko <roman@goteleport.com> Co-authored-by: daquinoaldo <aldd@bendingspoons.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.
@r0mant, thank you for taking care of this buddy PR. I was busy these days, and I didn't have the time to apply the requested changes yet. I appreciate.
@webvictim, do you have an ETA for the release?
- matchSnapshot: | ||
path: spec.template.spec |
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 changed it because spec.template.spec
is the spec
of Pods, while deployment annotations stay in the root object. I didn't experience issues with tests even before the change, but I felt this was incorrect, and I fixed it. Sorry for not specifying it.
…) (#12568) * feat(helm): add priorityClassName and extraLabels to kube-agent Signed-off-by: Roman Tkachenko <roman@goteleport.com> Co-authored-by: daquinoaldo <aldd@bendingspoons.com> Co-authored-by: daquinoaldo <aldd@bendingspoons.com>
Buddy PR for #12264.
@webvictim Could you take a look? I've applied your comments from #12264.
Fixes #8702.