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

install: fix ordering of operator resource block #16273

Merged
merged 1 commit into from Jun 2, 2021
Merged

install: fix ordering of operator resource block #16273

merged 1 commit into from Jun 2, 2021

Conversation

rkage
Copy link
Contributor

@rkage rkage commented May 21, 2021 •
edited

Signed-off-by: Nick M 4718+rkage@users.noreply.github.com

Fixes: #16272

Fix bug with Helm chart where a user could not enable BGP and set Operator resources.

@rkage rkage requested a review from a team as a code owner May 21, 2021 20:29
@rkage rkage requested review from a team and kkourt May 21, 2021 20:29
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 21, 2021
@rkage rkage requested a review from nathanjsweet May 21, 2021 20:29
@pchaigno pchaigno added area/helm Impacts helm charts and user deployment experience kind/bug This is a bug in the Cilium logic. needs-backport/1.10 release-note/bug This PR fixes an issue in a previous release of Cilium. labels May 21, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 21, 2021
@nathanjsweet nathanjsweet requested review from nathanjsweet and removed request for nathanjsweet and kkourt May 21, 2021 20:38
christarazi
christarazi requested changes May 22, 2021
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for the PR!

Could you rework the commit title to be something like, install: Fix ordering of operator resources block and then move the "fixes #16272" into a new line inside the commit msg body?

Also, in general the release note should be written so that a reader can understand the impact. For example, in this case it would be Fix bug with the Helm chart where a user couldn't enable BGP and set the Operator resources.

@maintainer-s-little-helper
Copy link

Commit f6762fbc06cd340c2043a0b4ab155cab8ecd43cf 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 the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 22, 2021
@maintainer-s-little-helper
Copy link

Commits f6762fbc06cd340c2043a0b4ab155cab8ecd43cf, e38165a40db5fbef115180b70d11106f2057317f do 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

@christarazi
Copy link
Member

christarazi commented May 22, 2021 •
edited

Seems like there have been some merge commits made in order to catch up to the master branch. Could you simply rebase your change on the latest master, without the need to merge branches into each other? That will also get rid of the sign-off warning as well.

@rkage
install: fix ordering of operator resource block
e66a835 
fixes #16272

Signed-off-by: Nick M <4718+rkage@users.noreply.github.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 May 23, 2021
@christarazi christarazi changed the title fix the order of blocks for bgp config mount and resources fixes #16272 install: fix ordering of operator resource block May 24, 2021
christarazi
christarazi approved these changes May 24, 2021
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment •
edited

Choose a reason for hiding this comment

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

Thanks! 🚀

@christarazi
Copy link
Member

christarazi commented May 24, 2021 •
edited

test-net-next

Edit: No need to run full CI as BGP is tested in net-next

@nathanjsweet nathanjsweet added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 2, 2021
@nathanjsweet nathanjsweet merged commit d31f029 into cilium:master Jun 2, 2021
@pchaigno pchaigno mentioned this pull request Jun 4, 2021
Merged
@aanm aanm mentioned this pull request Jun 16, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kkourt kkourt kkourt approved these changes

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@christarazi christarazi christarazi approved these changes

Assignees
No one assigned
Labels
area/helm Impacts helm charts and user deployment experience kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Helm template rendering error when specifying both bgp.enabled and operator.resources
6 participants
@rkage @christarazi @kkourt @nathanjsweet @pchaigno @aanm