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

Clarify repositories structure in install kubeadm document #43458

Merged
merged 1 commit into from
Oct 13, 2023
Merged

Clarify repositories structure in install kubeadm document #43458

merged 1 commit into from
Oct 13, 2023

Conversation

xmudrii
Copy link
Member

@xmudrii xmudrii commented Oct 12, 2023 •
edited
Loading

We received feedback that it's not clear enough that there's a dedicated package repository for each Kubernetes minor version (see #42810). This is a regression from #43407 and this PR is supposed to address this concern.

/assign @sftim @saschagrunert @cpanato @jeremyrickard
cc @kubernetes/release-engineering

@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 12, 2023
@xmudrii xmudrii mentioned this pull request Oct 12, 2023
Closed
cpanato
cpanato approved these changes Oct 12, 2023
View reviewed changes
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: dd1d52e2ce3881cd7f81ae2132a831ab23bd4de3

@netlify
Copy link

netlify bot commented Oct 12, 2023 •
edited
Loading

✅ Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 51f2625
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/65282cf04c2fb6000840ee88
😎 Deploy Preview https://deploy-preview-43458--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2023
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 12, 2023
@xmudrii
Copy link
Member Author

xmudrii commented Oct 12, 2023

I found a formatting issue in instructions for Red Had-based distros:

image

The issue is also present on k8s.io: https://kubernetes.io/docs/setup/production-environment/tools/kubeadm/install-kubeadm/

I fixed this issue in the latest commit.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 12, 2023
@xmudrii xmudrii requested a review from Affan-7 October 12, 2023 15:20
@xmudrii
Copy link
Member Author

xmudrii commented Oct 12, 2023

@Affan-7 I addressed your comments in the latest commit
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 12, 2023
sftim
sftim reviewed Oct 12, 2023
View reviewed changes
content/en/docs/setup/production-environment/tools/kubeadm/install-kubeadm.md
Comment on lines +18 to +24
This installation guide is for Kubernetes {{< skew currentVersion >}}. If you want to use a different
Kubernetes version, please refer to the following pages instead:

- [Installing kubeadm for Kubernetes {{< skew currentVersionAddMinor -1 "." >}}](https://v{{< skew currentVersionAddMinor -1 "-" >}}.docs.kubernetes.io/docs/setup/production-environment/tools/kubeadm/install-kubeadm/)
- [Installing kubeadm for Kubernetes {{< skew currentVersionAddMinor -2 "." >}}](https://v{{< skew currentVersionAddMinor -2 "-" >}}.docs.kubernetes.io/docs/setup/production-environment/tools/kubeadm/install-kubeadm/)
- [Installing kubeadm for Kubernetes {{< skew currentVersionAddMinor -3 "." >}}](https://v{{< skew currentVersionAddMinor -3 "-" >}}.docs.kubernetes.io/docs/setup/production-environment/tools/kubeadm/install-kubeadm/)
- [Installing kubeadm for Kubernetes {{< skew currentVersionAddMinor -4 "." >}}](https://v{{< skew currentVersionAddMinor -4 "-" >}}.docs.kubernetes.io/docs/setup/production-environment/tools/kubeadm/install-kubeadm/)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, this will break when v1.29 is released (the {{< skew currentVersionAddMinor -4 "." >}} link will not work). We could merge it now and track an important-soon issue to redo the logic here.

Also see #12303

Copy link
Contributor

@Affan-7 Affan-7 Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of using this list we can add a note similar to this one.

https://kubernetes.io/docs/tasks/tools/install-kubectl-linux/#install-kubectl-binary-with-curl-on-linux

This can be added before the installation steps.

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 copied this code from the Upgrading kubeadm clusters page but I see that it's also affected by the issue that you mentioned. Let's leave it as it is for now and I can create a follow-up issue once this PR is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using this list we can add a note similar to this one.

https://kubernetes.io/docs/tasks/tools/install-kubectl-linux/#install-kubectl-binary-with-curl-on-linux

We could, but let's not.

We want to help readers find the right docs for their version (we don't want to set an expectation that the Kubernetes website covers older versions). Let's avoid trapping ourselves into an expectationof backwards compatibility.

@xmudrii xmudrii requested a review from sftim October 12, 2023 15:39
@xmudrii
Copy link
Member Author

xmudrii commented Oct 12, 2023

@sftim Addressed your comments, PTAL again

sftim
sftim reviewed Oct 12, 2023
View reviewed changes
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

LGTM with the caveat from https://github.com/kubernetes/website/pull/43458/files#r1357008050

/lgtm

@sftim
Copy link
Contributor

sftim commented Oct 12, 2023

/lgtm

Ideally: squash to fewer commits

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 45cf00ed2fb5908a827973d0805e0ceb8d27f7d8

@xmudrii
Clarify repositories structure in install kubeadm document
51f2625 
Signed-off-by: Marko Mudrinić <mudrinic.mare@gmail.com>
@xmudrii
Copy link
Member Author

xmudrii commented Oct 12, 2023

@sftim Squashed the PR to one commit, PTAL again

@sftim
Copy link
Contributor

sftim commented Oct 12, 2023

Yep, still looks good to me (Prow has kept the lgtm label).

aj11anuj
aj11anuj reviewed Oct 13, 2023
View reviewed changes
Copy link
Member

@aj11anuj aj11anuj left a comment

Choose a reason for hiding this comment

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

For that skew version list thing, a separate issue can be raised later on. Rest everything
LGTM, 👍

@tengqm
Copy link
Contributor

tengqm commented Oct 13, 2023

Thanks.
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, tengqm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 13, 2023
@k8s-ci-robot k8s-ci-robot merged commit ea1abd7 into kubernetes:main Oct 13, 2023
6 checks passed
@windsonsea windsonsea mentioned this pull request Oct 13, 2023
Closed
@xmudrii xmudrii deleted the install-kubeadm-pkgs-minor branch October 13, 2023 08:49
This was referenced Oct 17, 2023
Merged
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sftim sftim sftim left review comments

@aj11anuj aj11anuj aj11anuj left review comments

@divya-mohan0209 divya-mohan0209 Awaiting requested review from divya-mohan0209

@natalisucks natalisucks Awaiting requested review from natalisucks

@cpanato cpanato Awaiting requested review from cpanato

@jeremyrickard jeremyrickard Awaiting requested review from jeremyrickard

@saschagrunert saschagrunert Awaiting requested review from saschagrunert

@Affan-7 Affan-7 Awaiting requested review from Affan-7

Assignees

@saschagrunert saschagrunert

@cpanato cpanato

@sftim sftim

@jeremyrickard jeremyrickard

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@xmudrii @k8s-ci-robot @sftim @tengqm @cpanato @Affan-7 @aj11anuj @saschagrunert @jeremyrickard