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

docs: Update description in egress gateway guide #23616

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

deepeshaburse
Copy link
Contributor

@deepeshaburse deepeshaburse commented Feb 7, 2023

Signed-off-by: Deepesha Burse deepesha.3007@gmail.com

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • 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
  • Thanks for contributing!

Fixes: #23256

Document that the `install-egress-gateway-routes` flag is only for EKS's ENI mode in egress gateway guide

@deepeshaburse deepeshaburse requested review from a team as code owners February 7, 2023 17:23
@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 Feb 7, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Feb 7, 2023
@pchaigno pchaigno requested review from jibi and removed request for YutaroHayakawa February 7, 2023 20:50
@pchaigno pchaigno 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 Feb 7, 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 Feb 7, 2023
Copy link
Member

@jibi jibi left a comment

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 we make it a warning please? You can find an example here:

.. warning::
The egress IP must be assigned to a network device on the node.

@deepeshaburse
Copy link
Contributor Author

Thanks for the PR!

Could we make it a warning please? You can find an example here:

.. warning::
The egress IP must be assigned to a network device on the node.

Sure, thanks for the review 😄

@maintainer-s-little-helper

This comment was marked as outdated.

@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 Feb 8, 2023
@maintainer-s-little-helper

This comment was marked as outdated.

@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 Feb 9, 2023
@deepeshaburse
Copy link
Contributor Author

@jibi I have updated the file, please review it.

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 good, thanks!

Can you please fix the formatting for the flag name and Helm value?

Can you also please squash your commits and make sure you keep only one Signed-off-by: tag in the description? Can you also extend that description by mentioning why this change is needed, please?

Documentation/network/egress-gateway.rst Outdated Show resolved Hide resolved
@maintainer-s-little-helper

This comment was marked as outdated.

@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 Feb 10, 2023
@maintainer-s-little-helper

This comment was marked as outdated.

@maintainer-s-little-helper

This comment was marked as outdated.

1 similar comment
@maintainer-s-little-helper

This comment was marked as outdated.

@maintainer-s-little-helper

This comment was marked as outdated.

@maintainer-s-little-helper

This comment was marked as outdated.

@deepeshaburse
Copy link
Contributor Author

@qmonnet I'm so sorry for how messy my PR has gotten. I was able to squash the first 3 commits into one but there are a few merges before the last commit (where I remove the warning). Is there any way to squash that too?

@qmonnet
Copy link
Member

qmonnet commented Feb 13, 2023

Yes there is, probably more than one way to do it. Please have a look at my example above.
And no need to apologise! Moving around commits with Git is not exactly the most intuitive thing.

@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 Feb 14, 2023
…EKS ENI mode

Signed-off-by: Deepesha Burse <deepesha.3007@gmail.com>
@deepeshaburse
Copy link
Contributor Author

@qmonnet It worked! Thank you so much for walking me through every step so patiently. I really appreciate it. I finally understood the entire process. 😄

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.

@qmonnet It worked! Thank you so much for walking me through every step so patiently. I really appreciate it. I finally understood the entire process. smile

Awesome to hear, thanks for all your efforts! Looks all good this time, and the CI test for documentation is passing.

@qmonnet qmonnet requested a review from jibi February 14, 2023 09:26
@jibi jibi added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 14, 2023
@nbusseneau
Copy link
Member

@deepeshaburse Thanks for your contribution!

@nbusseneau nbusseneau merged commit ff59a03 into cilium:master Feb 15, 2023
@julianwiedmann julianwiedmann mentioned this pull request Jul 11, 2023
1 task
@julianwiedmann julianwiedmann added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Jul 11, 2023
@gandro gandro mentioned this pull request Jul 17, 2023
6 tasks
@gandro gandro added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. feature/egress-gateway Impacts the egress IP gateway feature. kind/community-contribution This was a contribution made by a community member. 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
None yet
Development

Successfully merging this pull request may close these issues.

Document that install-egress-gateway-routes is specific to EKS
7 participants