-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Remove docker installation steps from Administer Cluster docs #31059
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
||
Install Docker | ||
Instructions to do so are available at [Install Docker Engine - Enterprise on Windows Servers](https://docs.microsoft.com/en-us/virtualization/windowscontainers/quick-start/set-up-environment?tabs=Windows-Server#install-docker). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tab actually starts on line 150 and continues to line 162.
Instead of removing one of the 2 tabs, I would tell readers that they must install a container runtime and list some compatible ones. Rather than providing the instructions, we hyperlink to them.
For example:
- containerd
- Mirantis Container Runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also link to https://github.com/kubernetes-sigs/sig-windows-tools/ to let readers know that there might be a helper script for your preferred runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to this suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@championshuttler following up here, are you willing to incorporate Tim's suggestion? thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL at the suggestions provided by Tim
|
||
Install Docker | ||
Instructions to do so are available at [Install Docker Engine - Enterprise on Windows Servers](https://docs.microsoft.com/en-us/virtualization/windowscontainers/quick-start/set-up-environment?tabs=Windows-Server#install-docker). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to this suggestion
/assign @celestehorgan |
@@ -146,20 +146,6 @@ All code snippets in Windows sections are to be run in a PowerShell environment | |||
with elevated permissions (Administrator) on the Windows worker node. | |||
{{< /note >}} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have a PR that overlaps with the kubeadm / windows install sections in this PR: ideally this PR should be rebased on top of mine, if needed, as mine touches all kubeadm docs related to the Docker change. EDIT: also this PR seems to be against "main", and should be against dev-1.24 instead? |
@neolit123 the easiest thing to do overall is if you can add a commit to #31309 that makes the appropriate changes, and list @championshuttler as co-author. Would you have time to do that? If we pick that approach, we can close this PR. |
also looks like |
/hold |
Since
and also Netlify build fails, |
@jihoon-seo: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Part of #30970