Skip to content
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.

Sign up for GitHub

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

Jump to bottom

helm: renamed podsAnnotations variable into podAnnotations #13458

Merged
merged 2 commits into from
Oct 13, 2020
Merged

helm: renamed podsAnnotations variable into podAnnotations #13458

merged 2 commits into from
Oct 13, 2020

Conversation

mvisonneau
Copy link
Contributor

This change follows-up onto #12189 (comment)
It concerns the agent and operator charts

@mvisonneau mvisonneau requested review from a team as code owners October 8, 2020 10:24
@mvisonneau mvisonneau requested a review from a team October 8, 2020 10:24
@maintainer-s-little-helper
Copy link

Commit f8d75501df61c9711972715e83962357a784d2a6 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 8, 2020
@sayboras
Copy link
Member

sayboras commented Oct 8, 2020 •
edited

@mvisonneau thanks, seems like this one is coverred in #13259 as well. If it is not so urgent, I think we can wait for a little, as #13259 seems pretty closed from my understanding :)

EDIT: Correct typo from my side :(

@mvisonneau
Copy link
Contributor Author

oh cool, sure feel free to do whatever you want with this PR then! :)

@kaworu kaworu added the dont-merge/blocked Another PR must be merged before this one. label Oct 8, 2020
@dominik318
Copy link

@sayboras this PR is required for Cilium DataDog integration which is somewhat urgent for many people. It would be fantastic if we could back-port this to 1.8. This PR is fully backwards compatible.

@sayboras sayboras added area/helm Impacts helm charts and user deployment experience release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Oct 8, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 8, 2020
@sayboras
Copy link
Member

sayboras commented Oct 8, 2020 •
edited

this PR is required for Cilium DataDog integration which is somewhat urgent for many people. It would be fantastic if we could back-port this to 1.8. This PR is fully backwards compatible.

Thanks for clarification, I am big fan of datadog 🥇

@sayboras sayboras requested review from a team October 8, 2020 21:32
christarazi
christarazi approved these changes Oct 8, 2020
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! We'll need to wait until the big Helm refactor is merged.

@sayboras I think because the original change never made it into a release, we could probably skip mentioning it. It seems like this PR is just amending the previous change. (Unless I've missed something.)

sayboras
sayboras approved these changes Oct 8, 2020
View reviewed changes
Copy link
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💯

kaworu
kaworu requested changes Oct 9, 2020
View reviewed changes
Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is my understanding:

#13259 does include this fix, so the issue will be solved as soon as #13259 is merged into master.

There is a need to have this fix in v1.8. Since this PR won't be merged into master, it could be re-targeted to be merged into the v1.8 branch. Alternatively, this PR could be closed and another PR could be opened against v1.8.

@joestringer
Copy link
Member

Thanks for clarifying @kaworu , confirmed. @mvisonneau would you mind retargeting these changes for the v1.8 branch?

@joestringer joestringer mentioned this pull request Oct 9, 2020
Merged
@mvisonneau mvisonneau changed the base branch from master to v1.8 October 10, 2020 21:04
@mvisonneau mvisonneau requested review from a team as code owners October 10, 2020 21:04
@mvisonneau
helm: configurable annotations for agent and operator pods
b92b7c6 
Signed-off-by: Maxime VISONNEAU <maxime.visonneau@gmail.com>
@mvisonneau
helm: renamed podsAnnotations variable into podAnnotations
548cf75 
This change follows-up onto cilium#12189 (comment)

Signed-off-by: Maxime VISONNEAU <maxime.visonneau@gmail.com>
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 10, 2020
@mvisonneau
Copy link
Contributor Author

there you go @joestringer! 🙇

@sayboras sayboras removed the dont-merge/blocked Another PR must be merged before this one. label Oct 10, 2020
@sayboras sayboras requested a review from kaworu October 10, 2020 22:27
kaworu
kaworu approved these changes Oct 12, 2020
View reviewed changes
Copy link
Member

@kaworu kaworu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@kaworu
Copy link
Member

kaworu commented Oct 12, 2020

test-me-please

@gandro
Copy link
Member

gandro commented Oct 12, 2020

test-backport-1.8

@aanm aanm merged commit 7ad2c73 into cilium:v1.8 Oct 13, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 13, 2020
@christarazi christarazi mentioned this pull request Oct 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@kaworu kaworu kaworu approved these changes

@rolinh rolinh rolinh approved these changes

@christarazi christarazi christarazi approved these changes

@sayboras sayboras sayboras approved these changes

Assignees

@seanmwinn seanmwinn

Labels
area/helm Impacts helm charts and user deployment experience ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@mvisonneau @sayboras @dominik318 @joestringer @kaworu @gandro @rolinh @christarazi @aanm @seanmwinn