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: merge Alibaba install guide into quick install guide #21581

Merged
merged 3 commits into from
Oct 18, 2022

Conversation

yoyo-go
Copy link
Contributor

@yoyo-go yoyo-go commented Oct 4, 2022

According to the slack thread with @joestringer , we diecide to merge the Alibaba ACK install guide into the Cilium quick install guide, as a new tab named Alibaba ACK.

@qmonnet @lizrice @xmulligan Please let me know if you have any comment on it.

@yoyo-go yoyo-go requested review from a team as code owners October 4, 2022 15:59
@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 Oct 4, 2022
@yoyo-go yoyo-go changed the title Merge Alibaba install guide into quick install guide docs: merge Alibaba install guide into quick install guide Oct 4, 2022
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Oct 4, 2022
@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 Oct 4, 2022
@qmonnet qmonnet added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. kind/docs-reorganization Documentation reorganisation, as part of GSoD (Google Season of Documentation) gsod-22 All issues and PRs done in the context of the Google Season of Documentation 2022 labels Oct 5, 2022
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, thank you!

Two minor suggestions below.

Documentation/gettingstarted/k8s-install-default.rst Outdated Show resolved Hide resolved
Documentation/installation/alibabacloud-eni.rst Outdated Show resolved Hide resolved
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 pending Quentin's suggestions.

Signed-off-by: Yoyo Wu <yoyo19980720@126.com>
Signed-off-by: Yoyo Wu <yoyo19980720@126.com>
@yoyo-go yoyo-go force-pushed the docs-restructure-alibaba-merge branch from 6d4403b to 2a7f5a9 Compare October 16, 2022 14:25
@yoyo-go
Copy link
Contributor Author

yoyo-go commented Oct 16, 2022

Hi @qmonnet Sorry for the late cause I am sick for a week, I have updated this PR per your comments, But @joestringer suggests us to merge the Alibaba one into the Installation using Helm page. How do you think about this idea?

@qmonnet
Copy link
Member

qmonnet commented Oct 17, 2022

@yoyo-go Can you please provide some context, what's the motivation? I suppose you refer to this discussion and that this is because Alibaba does not support install through the Cilium CLI? In that case, yes, it's probably better to move it to the Helm page instead.

…guide

Signed-off-by: Yoyo Wu <yoyo19980720@126.com>
@yoyo-go
Copy link
Contributor Author

yoyo-go commented Oct 18, 2022

@qmonnet Yes that is the discussion, and I have updated the Helm page, and add a ref in our quick installation guide. Please have a look and let me know if this PR is ready.

@qmonnet
Copy link
Member

qmonnet commented Oct 18, 2022

Thanks! But wasn't Joe's comment about adding the instructions to the Helm page only?
Cc @joestringer can you please have a quick look?

@joestringer
Copy link
Member

@qmonnet What's the question?

At a glance, the changes look roughly right:

  • ACK is now listed under the helm install
  • There is a section for ACK under the cilium install. Ideally this would just say to install via cilium CLI, but I guess we have not validated that this works so it's fine by me that it says to use the helm install approach instead.
  • Remove the cloud install page because the current version implies that we don't support other clouds. (The alternatives here are either list every cloud there, or remove the page. I'm OK with removing the page like is done here)

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.

Apologies, I got confused when reviewing the PR earlier today and thought we were adding the instructions both to the Helm page and the Quick install guide (where we in fact redirect to the Helm page). Sorry for the noise and thanks for taking a look.

All good then, let's get this merged! Just waiting for the Travis build just in case.

@qmonnet qmonnet merged commit 11dbe54 into cilium:master Oct 18, 2022
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. gsod-22 All issues and PRs done in the context of the Google Season of Documentation 2022 kind/docs-reorganization Documentation reorganisation, as part of GSoD (Google Season of Documentation) 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.

None yet

4 participants