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.

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

Helm: Improved description for tunnel, tunnelProtocol, routingMode flags #27926

Conversation

PhilipSchmid
Copy link
Contributor

@PhilipSchmid PhilipSchmid commented Sep 4, 2023

Please ensure your pull request adheres to the following guidelines:

Helm: Improved description for tunnel, tunnelProtocol, routingMode flags

Internal Slack discussion: https://isovalent.slack.com/archives/C01742ANV9P/p1693834569477619

@PhilipSchmid PhilipSchmid requested review from a team as code owners September 4, 2023 14:26
@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 Sep 4, 2023
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Travis is failing with a legitimate error:

HINT: to fix this, run 'make -C Documentation update-helm-values'

Otherwise this looks good to me from a Helm perspective.

@PhilipSchmid PhilipSchmid force-pushed the pr/philip/helm_description_for_tunnelProtocol_routingMode branch from c35453d to fd3dd22 Compare September 4, 2023 15:18
@PhilipSchmid
Copy link
Contributor Author

Ugh, I forgot about this, thanks @gandro! I just run this command and git amend'ed the generated file.

@gandro gandro added release-note/misc This PR makes changes that have no direct user impact. area/helm Impacts helm charts and user deployment experience labels Sep 4, 2023
@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 Sep 4, 2023
@gandro
Copy link
Member

gandro commented Sep 14, 2023

Restarting both

@gandro
Copy link
Member

gandro commented Sep 14, 2023

/test

@PhilipSchmid PhilipSchmid force-pushed the pr/philip/helm_description_for_tunnelProtocol_routingMode branch from fd3dd22 to 3ee196c Compare September 28, 2023 06:29
- Improved description for tunnel, tunnelProtocol, routingMode flags to make it clearer for users to know the possible values.
- Added deprecation note for the tunnel flag to be in line with the v1.14.0 release notes and cilium#24561.

Signed-off-by: Philip Schmid <philip.schmid@isovalent.com>
@PhilipSchmid PhilipSchmid force-pushed the pr/philip/helm_description_for_tunnelProtocol_routingMode branch from 3ee196c to 53d4125 Compare September 28, 2023 06:42
@PhilipSchmid PhilipSchmid requested a review from a team as a code owner September 28, 2023 06:42
@PhilipSchmid
Copy link
Contributor Author

Rebased & fixed two word spellings.

@gandro, @nathanjsweet do you think we could merge this PR anytime soon? Let's please backport this to 1.14. For 1.15, tunnel should be removed anytime soon, I guess.

@gandro
Copy link
Member

gandro commented Sep 28, 2023

The main reason why this was not yet merged is that there is an outstanding review request for docs by @qmonnet - Quentin, mind taking a look? Otherwise I think we can merge without docs approval, since the changes mainly affect the Helm README.

Let's also run CI and then this should be mergable.

@gandro
Copy link
Member

gandro commented Sep 28, 2023

/test

@gandro gandro added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Sep 28, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.3 Sep 28, 2023
@gandro
Copy link
Member

gandro commented Sep 28, 2023

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks all good, thanks

@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 Sep 28, 2023
@lmb lmb merged commit dde4722 into cilium:main Sep 28, 2023
60 of 61 checks passed
@sayboras sayboras mentioned this pull request Oct 2, 2023
3 tasks
@sayboras sayboras added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Oct 2, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.3 Oct 2, 2023
@aanm aanm added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Oct 3, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.14 to Backport done to v1.14 in 1.14.3 Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Impacts helm charts and user deployment experience backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.14.3
Backport done to v1.14
Development

Successfully merging this pull request may close these issues.

None yet

7 participants