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

doc: Documented incompatibility of EgressGW and kvstore #26139

Merged
merged 1 commit into from Jul 5, 2023
Merged

doc: Documented incompatibility of EgressGW and kvstore #26139

merged 1 commit into from Jul 5, 2023

Conversation

PhilipSchmid
Copy link
Contributor

Egress gateway isn't compatible with identity allocation mode kvstore. Hence, I documented this limitation.

Please ensure your pull request adheres to the following guidelines:

  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.

Fixes: #issue-number

doc: Documented incompatibility of EgressGW and kvstore

@PhilipSchmid PhilipSchmid requested review from a team as code owners June 12, 2023 20:33
@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 Jun 12, 2023
zacharysarah
zacharysarah requested changes Jun 13, 2023
View reviewed changes
Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

@PhilipSchmid Some edits for clarity. Please verify that my revision of settings for K8s egress gateway is correct.

Documentation/network/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/network/egress-gateway.rst Outdated Show resolved Hide resolved
@jibi jibi added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. feature/egress-gateway Impacts the egress IP gateway feature. labels Jun 13, 2023
@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 Jun 13, 2023
@PhilipSchmid PhilipSchmid force-pushed the pr/philip/doc_egressgw_kvstore_limitation branch from 7d29acc to b4bc7cb Compare June 13, 2023 08:28
zacharysarah
zacharysarah requested changes Jun 21, 2023
View reviewed changes
Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

@PhilipSchmid Minor nits only, good work. ✨

Documentation/network/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/network/egress-gateway.rst Outdated Show resolved Hide resolved
@PhilipSchmid PhilipSchmid force-pushed the pr/philip/doc_egressgw_kvstore_limitation branch from c25c9e9 to c6e520d Compare June 21, 2023 07:54
@PhilipSchmid
Copy link
Contributor Author

Awesome, thanks for the detailed review, @zacharysarah! This truly helps to improve our doc quality.

I've now applied your two last suggestions and squashed them into 1 commit. I think it should now be ready to be merged. It looks like this:
image

zacharysarah
zacharysarah requested changes Jun 21, 2023
View reviewed changes
Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

@PhilipSchmid Thanks for the update! One more quick update to un-bullet paragraphs, then it's ready. 🙏🏻

Documentation/network/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/network/egress-gateway.rst Outdated Show resolved Hide resolved
Documentation/network/egress-gateway.rst Outdated Show resolved Hide resolved
@PhilipSchmid
doc: Documented incompatibility of EgressGW and kvstore
5e9666b 
Signed-off-by: Philip Schmid <philip.schmid@isovalent.com>
@PhilipSchmid PhilipSchmid force-pushed the pr/philip/doc_egressgw_kvstore_limitation branch from 12c6416 to 5e9666b Compare June 22, 2023 06:45
@PhilipSchmid
Copy link
Contributor Author

@zacharysarah Thanks for your suggested changes to remove the bullet points. I've applied them, and now it looks like this:
image

zacharysarah
zacharysarah approved these changes Jun 22, 2023
View reviewed changes
Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

@PhilipSchmid Thank you so much! LGTM

@julianwiedmann
Copy link
Member

@PhilipSchmid is there anything that still needs to be done here before merging? :)

@PhilipSchmid
Copy link
Contributor Author

@julianwiedmann Nope, this PR can be merged 😄. (I don't have the permission to do so)

@julianwiedmann julianwiedmann added needs-backport/1.11 needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Jul 3, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.13.5 Jul 3, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.12.12 Jul 3, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.11.19 Jul 3, 2023
@julianwiedmann julianwiedmann added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 3, 2023
@borkmann borkmann merged commit dcc0a0e into cilium:main Jul 5, 2023
30 of 39 checks passed
@PhilipSchmid PhilipSchmid deleted the pr/philip/doc_egressgw_kvstore_limitation branch July 5, 2023 06:55
@joamaki joamaki mentioned this pull request Jul 5, 2023
Merged
23 tasks
@joamaki joamaki 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 Jul 5, 2023
@joamaki joamaki mentioned this pull request Jul 5, 2023
Merged
1 task
@joamaki joamaki added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Jul 5, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.13 in 1.13.5 Jul 5, 2023
@joamaki joamaki mentioned this pull request Jul 6, 2023
Merged
1 task
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.12 in 1.12.12 Jul 6, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Needs backport from main in 1.11.19 Jul 6, 2023
@joamaki joamaki added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Jul 7, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.13 to Backport done to v1.13 in 1.13.5 Jul 7, 2023
@jibi jibi 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 Jul 10, 2023
@julianwiedmann julianwiedmann added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.12 labels Jul 11, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.12 to Backport done to v1.12 in 1.12.12 Jul 11, 2023
@joestringer joestringer mentioned this pull request Jul 17, 2023
Merged
This was referenced Jul 26, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jibi jibi jibi approved these changes

@zacharysarah zacharysarah zacharysarah approved these changes

@aspsk aspsk aspsk approved these changes

Assignees
No one assigned
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. feature/egress-gateway Impacts the egress IP gateway feature. 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.12.12
Backport done to v1.12
1.13.5
Backport done to v1.13
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@PhilipSchmid @julianwiedmann @jibi @zacharysarah @aspsk @borkmann @joamaki