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

Rework Quick & Helm Installation Guide #15695

Merged
merged 3 commits into from
Apr 20, 2021
Merged

Rework Quick & Helm Installation Guide #15695

merged 3 commits into from
Apr 20, 2021

Conversation

tgraf
Copy link
Member

@tgraf tgraf commented Apr 14, 2021 •
edited

Major rework of quick & helm install guides. Most guides are consolidated into two main ones.

Rendered preview:
https://deploy-preview-15695--cilium-doc.netlify.app/

@tgraf tgraf added the area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. label Apr 14, 2021
@tgraf tgraf requested review from a team as code owners April 14, 2021 17:12
@tgraf tgraf requested review from a team April 14, 2021 17:12
@tgraf tgraf requested a review from a team as a code owner April 14, 2021 17:12
@tgraf tgraf requested a review from christarazi April 14, 2021 17:12
@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 Apr 14, 2021
@tgraf tgraf requested a review from ti-mo April 14, 2021 17:12
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Apr 14, 2021
@tgraf tgraf requested a review from joestringer April 14, 2021 17:12
@tgraf tgraf marked this pull request as draft April 14, 2021 17:12
@tgraf tgraf added the release-note/major This PR introduces major new functionality to Cilium. label Apr 14, 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 Apr 14, 2021
@tgraf tgraf force-pushed the pr/tgraf/new-quick-install branch from 275309a to 09da4bf Compare April 15, 2021 08:41
@tgraf
doc: New quick install guide
72a80c1 
This commit reworks the quick installation guides to use the Cilium CLI.
This allows to completely remove the quick-install.yaml.

The Helm installation guides are kept and moved into a new "Advanced
Installation" section.

This also allows to standardize the Observability instructions and move
it into a single guide split into Setup, CLI, and UI.

In order to really simplify, the only remaining guide that was using the
cilium namespace for installation is also changed to use kube-system so
all follow-up guides can be kept the same.

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf
k8s: Remove quick-install.yaml generation logic
4991138 
As we no longer depend on the various quick-install YAML files, we can
remove all the logic to generate them.

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf tgraf force-pushed the pr/tgraf/new-quick-install branch from 09da4bf to 01b8fee Compare April 19, 2021 11:26
@tgraf tgraf marked this pull request as ready for review April 19, 2021 11:27
@tgraf tgraf requested review from a team and twpayne April 19, 2021 11:27
@tgraf tgraf changed the title Rework Quick Installation Guide Rework Quick & Helm Installation Guide Apr 19, 2021
@tgraf tgraf force-pushed the pr/tgraf/new-quick-install branch from 01b8fee to d56ded7 Compare April 19, 2021 11:35
@tgraf tgraf added this to the 1.10.0-rc.1 milestone Apr 19, 2021
@tgraf tgraf mentioned this pull request Apr 19, 2021
Closed
nbusseneau
nbusseneau reviewed Apr 19, 2021
View reviewed changes
Documentation/gettingstarted/k8s-install-default.rst

.. tabs::

.. group-tab:: GKE
Copy link
Member

Choose a reason for hiding this comment

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

[Nit] For consistency, we should rename tabs for GKE/AKS/EKS to include the platform names, as in the next set of tabs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unlike the tabs below, these are actually specific to GKE, EKS, AKS, and so on.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I'm not sure I follow 🤔
Does that mean there is a difference between the GKE tab highlighted above and the GCP/GKE tab present in the next set of tabs (line 134 in same file)?

.. group-tab:: GCP/GKE

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the "GKE" tab in create cluster is creating a GKE cluster. The "GCP/GKE" tab in "Install Cilium" will function for both GKE and self-managed clusters in GCP. I'm OK to also label GCP above if you think that's less confusing overall.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see! I think I got mostly confused by the fact the GCP/GKE-like tabs explicitly only mention provider-managed clusters (e.g. Install Cilium into the GKE cluster:), so I did not think the docs were targeting self-managed GCP clusters.

Perhaps we ought to rephrase some parts a bit differently for self-managed clusters.

christarazi
christarazi approved these changes Apr 19, 2021
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM, one optional minor comment on minikube target

contrib/scripts/minikube.sh
Comment on lines -16 to -22
cp "install/kubernetes/quick-install.yaml" /tmp/cilium-minikube.yaml

sed -i 's|latest|dev|g' /tmp/cilium-minikube.yaml
sed -i 's|docker.io/||g' /tmp/cilium-minikube.yaml
sed -i 's|imagePullPolicy: Always|imagePullPolicy: Never|g' /tmp/cilium-minikube.yaml

kubectl create -f /tmp/cilium-minikube.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Instead of removing this script and the minikube target as there might be developers who depend on it (I'm not sure who though), maybe we can replace the highlighted lines with the cilium-cli?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've asked @mrostecki whether the script is still needed.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't use the script lately.

The question is if anyone finds it useful. If not, I'm fine with removing it. Otherwise can follow up with switching to cilium-cli in it.

@tgraf
Add new unified Helm guide
b6ec84c 
Consolidate most separate installation guides covering Helm-based
installation into a single guide with tabs for the different platforms.

Some guides are left intact which are either quite special or relatively
uncommon.

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf tgraf force-pushed the pr/tgraf/new-quick-install branch from d56ded7 to b6ec84c Compare April 19, 2021 19:48
@tgraf tgraf merged commit 3662560 into cilium:master Apr 20, 2021
1.10.0 automation moved this from In progress to Done Apr 20, 2021
@tgraf tgraf deleted the pr/tgraf/new-quick-install branch April 20, 2021 09:35
nbusseneau
nbusseneau reviewed Apr 20, 2021
View reviewed changes
Documentation/gettingstarted/k8s-install-default.rst
Comment on lines +110 to +112
You can install Cilium on any Kubernetes cluster. If you already have a
Kubernetes cluster, skip this step. If you want to create a cluster, pick one
of the options below:
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed the bit about skipping the step if a cluster is already available should probably be moved to the previous section.

@gandro gandro mentioned this pull request Apr 20, 2021
Merged
This was referenced Apr 28, 2021
Closed
Merged
@nbusseneau nbusseneau mentioned this pull request Apr 12, 2022
Merged
@pchaigno pchaigno mentioned this pull request May 10, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jibi jibi jibi left review comments

@errordeveloper errordeveloper errordeveloper left review comments

@ti-mo ti-mo ti-mo approved these changes

@nbusseneau nbusseneau nbusseneau left review comments

@aanm aanm aanm left review comments

@vadorovsky vadorovsky vadorovsky left review comments

@christarazi christarazi christarazi approved these changes

@michi-covalent michi-covalent Awaiting requested review from michi-covalent

@qmonnet qmonnet Awaiting requested review from qmonnet

@joestringer joestringer Awaiting requested review from joestringer

@twpayne twpayne Awaiting requested review from twpayne

Assignees

@twpayne twpayne

@errordeveloper errordeveloper

@joestringer joestringer

@ti-mo ti-mo

@christarazi christarazi

@qmonnet qmonnet

@michi-covalent michi-covalent

Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/major This PR introduces major new functionality to Cilium.
Projects
No open projects
1.10.0
Done
Milestone
1.10.0-rc.1
Development

Successfully merging this pull request may close these issues.

None yet