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: improve the aws-cni chaining page #15979

Merged
merged 1 commit into from May 11, 2021
Merged

docs: improve the aws-cni chaining page #15979

merged 1 commit into from May 11, 2021

Conversation

bmcustodio
Copy link
Contributor

@bmcustodio bmcustodio commented May 3, 2021

Improve the AWS VPC CNI plugin chaining page. Also, make use of the Cilium CLI to check for the status of the installation and for performing the connectivity test.

docs: improve the aws-cni chaining page

@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 3, 2021
@nbusseneau nbusseneau self-requested a review May 3, 2021 10:05
@maintainer-s-little-helper
Copy link

Commit 80db1e77d1cf93908dd92b3e1e4d1f6f7e24b6c3 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 5, 2021
@maintainer-s-little-helper
Copy link

Commit 80db1e77d1cf93908dd92b3e1e4d1f6f7e24b6c3 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

@bmcustodio bmcustodio marked this pull request as ready for review May 5, 2021 15:49
@bmcustodio bmcustodio requested review from a team as code owners May 5, 2021 15:49
@bmcustodio bmcustodio requested review from tgraf and qmonnet May 5, 2021 15:49
@aanm aanm removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 5, 2021
@maintainer-s-little-helper
Copy link

Commit 237d07f16a85d91d3d701f574de7d7406dbaf662 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 6, 2021
@maintainer-s-little-helper
Copy link

Commit 237d07f16a85d91d3d701f574de7d7406dbaf662 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

2 similar comments
@maintainer-s-little-helper
Copy link

Commit 237d07f16a85d91d3d701f574de7d7406dbaf662 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
Copy link

Commit 237d07f16a85d91d3d701f574de7d7406dbaf662 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
Copy link

Commits 237d07f16a85d91d3d701f574de7d7406dbaf662, 9c58e35f8c4581fa33377f598bb504c0f249f0b9, c456a183edb5f93cbac2376d4b5fe1d381b92a8e, 4028593aaf1a1489a69d3835b26ca8dc7996bb73 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

@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 6, 2021
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.

Contents look good to me.

On code formatting: Can you please avoid splitting the blocks? I see two consecutive export in section “Enabling security groups for pods (EKS)”, but I don't understand why they can't be in the same code snippet. Further down, please do not split between commands and output. It's not a bad option, but it's not consistent with what we do everywhere else in the documentation.

Regarding the directives: prefer .. code-block:: to .. code. When the snippet is more about commands to type in the console, with their output, use .. code-block:: shell-session. The bash language should be for larger bash snippets, like the for loop you have to find the pods that need to be restarted. When no syntax highlighting should be performed (i.e. no command or code in the block), the marker for a literal block :: should be used.

I understand we should document this somewhere, I started a doc but didn't have time to make a lot of progress on it.

Documentation/gettingstarted/cni-chaining-aws-cni.rst Outdated Show resolved Hide resolved
@qmonnet qmonnet added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. needs-backport/1.10 release-note/misc This PR makes changes that have no direct user impact. labels May 6, 2021
@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 May 6, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.0-rc2 May 6, 2021
@maintainer-s-little-helper
Copy link

Commit 86abb55793fdfdd515a01e336e7ebdce5a52f433 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

@bmcustodio bmcustodio added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 11, 2021
@bmcustodio
Copy link
Contributor Author

Marking as ready to merge as it was approved before and the only change since previous reviews was essentially masqueradeenableIPv4Masquerade.

@jrajahalme jrajahalme merged commit 3b350ce into cilium:master May 11, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.0-rc2 May 12, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.0-rc2 May 12, 2021
@bmcustodio bmcustodio deleted the bmcustodio-aws-cni-chaining-and-cilium-cli branch May 12, 2021 07:54
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.0-rc2 May 13, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.0-rc2 May 13, 2021
nbusseneau added a commit to nbusseneau/cilium that referenced this pull request May 20, 2021
In cilium#15979, the old `k8s-install-validate.rst` and `k8s-install-connectivity-test.rst`
were refactored to use the CLI, which broke the flow of several pages:
in particular, all installations based on Helm were half-broken due to
referencing Cilium CLI commands when the user was never instructed to
install it.

This commit moves all CLI-related operations to independent `cli-*.rst`,
and then refactors `k8s-install-validate.rst` to have both the new CLI
status check and connectivity test and the older manual status check and
connectivity test.

It then refactors CLI-based installation guides to use the `cli-*.rst`
in the order that makes the most sense for each page.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
twpayne pushed a commit that referenced this pull request May 25, 2021
In #15979, the old `k8s-install-validate.rst` and `k8s-install-connectivity-test.rst`
were refactored to use the CLI, which broke the flow of several pages:
in particular, all installations based on Helm were half-broken due to
referencing Cilium CLI commands when the user was never instructed to
install it.

This commit moves all CLI-related operations to independent `cli-*.rst`,
and then refactors `k8s-install-validate.rst` to have both the new CLI
status check and connectivity test and the older manual status check and
connectivity test.

It then refactors CLI-based installation guides to use the `cli-*.rst`
in the order that makes the most sense for each page.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
qmonnet pushed a commit to qmonnet/cilium that referenced this pull request May 26, 2021
[ upstream commit 497ac33 ]

In cilium#15979, the old `k8s-install-validate.rst` and `k8s-install-connectivity-test.rst`
were refactored to use the CLI, which broke the flow of several pages:
in particular, all installations based on Helm were half-broken due to
referencing Cilium CLI commands when the user was never instructed to
install it.

This commit moves all CLI-related operations to independent `cli-*.rst`,
and then refactors `k8s-install-validate.rst` to have both the new CLI
status check and connectivity test and the older manual status check and
connectivity test.

It then refactors CLI-based installation guides to use the `cli-*.rst`
in the order that makes the most sense for each page.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
qmonnet pushed a commit to qmonnet/cilium that referenced this pull request Jun 4, 2021
[ upstream commit 497ac33 ]

In cilium#15979, the old `k8s-install-validate.rst` and `k8s-install-connectivity-test.rst`
were refactored to use the CLI, which broke the flow of several pages:
in particular, all installations based on Helm were half-broken due to
referencing Cilium CLI commands when the user was never instructed to
install it.

This commit moves all CLI-related operations to independent `cli-*.rst`,
and then refactors `k8s-install-validate.rst` to have both the new CLI
status check and connectivity test and the older manual status check and
connectivity test.

It then refactors CLI-based installation guides to use the `cli-*.rst`
in the order that makes the most sense for each page.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
nathanjsweet pushed a commit that referenced this pull request Jun 4, 2021
[ upstream commit 497ac33 ]

In #15979, the old `k8s-install-validate.rst` and `k8s-install-connectivity-test.rst`
were refactored to use the CLI, which broke the flow of several pages:
in particular, all installations based on Helm were half-broken due to
referencing Cilium CLI commands when the user was never instructed to
install it.

This commit moves all CLI-related operations to independent `cli-*.rst`,
and then refactors `k8s-install-validate.rst` to have both the new CLI
status check and connectivity test and the older manual status check and
connectivity test.

It then refactors CLI-based installation guides to use the `cli-*.rst`
in the order that makes the most sense for each page.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
Signed-off-by: Quentin Monnet <quentin@isovalent.com>
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. 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.10.0-rc2
Backport done to v1.10
Development

Successfully merging this pull request may close these issues.

None yet

7 participants