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

Fixes commands not working without sudo #17139

Merged
merged 1 commit into from
Oct 28, 2019

Conversation

vanou
Copy link
Contributor

@vanou vanou commented Oct 23, 2019

In doc of kubeadm installation, there are commands which wont work without sudo regarding to Ubuntu/Debian instruction.
However, other docs related to kubeadm under content/en/docs/setup/production-environment have commands with sudo.

This commit fixes this inconsistency.

In doc of kubeadm installation, there are commands which wont work
without sudo regarding to Ubuntu/Debian instruction. However, other
docs related to kubeadm under
content/en/docs/setup/production-environment have commands with sudo.

This commit fixes this inconsistency.
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Oct 23, 2019
@netlify
Copy link

netlify bot commented Oct 23, 2019

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 7a4970a

https://deploy-preview-17139--kubernetes-io-master-staging.netlify.com

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.

It seems a bit odd to document using sudo for just one of the OSs. If the change is good for Debian it seems like it'd be good for Fedora as well.

Separately, but I'm going to mention it, I don't like sudo. I'm personally wary of nudging readers towards a particular tool. That's my opinion and not an outright request to change. If other reviewers have similar feelings then that's worth taking into account.

sudo update-alternatives --set iptables /usr/sbin/iptables-legacy
sudo update-alternatives --set ip6tables /usr/sbin/ip6tables-legacy
sudo update-alternatives --set arptables /usr/sbin/arptables-legacy
sudo update-alternatives --set ebtables /usr/sbin/ebtables-legacy
Copy link
Contributor

Choose a reason for hiding this comment

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

Just the Debian tab? Line 77 has update-alternatives and that also requires superuser access.

update-alternatives --set ip6tables /usr/sbin/ip6tables-legacy
update-alternatives --set arptables /usr/sbin/arptables-legacy
update-alternatives --set ebtables /usr/sbin/ebtables-legacy
sudo update-alternatives --set iptables /usr/sbin/iptables-legacy
Copy link
Contributor

Choose a reason for hiding this comment

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

You could add a comment saying that these commands need superuser access, eg sudo.
There are alternatives to sudo (and I use some of them).

@bradtopol
Copy link
Contributor

Assigning @sftim since he's on top of this. Thanks Tim
/assign @sftim

@zparnold
Copy link
Member

/approve
pending @sftim 's lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zparnold

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 23, 2019
@neolit123
Copy link
Member

neolit123 commented Oct 23, 2019

i'm mostly -1 on adding sudo to the kubeadm docs. i don't think we have it the rest of our docs.
much better to add a note to execute commands as root.

@vanou
Copy link
Contributor Author

vanou commented Oct 24, 2019

Thanks @sftim , @bradtopol , @zparnold & @neolit123 !

My main concerns are

  • inconsistency of using sudo across kubeadm doc (i.e. here&here with sudo but here without) and
  • no indication of where root privilege is required. (or reader who is relatively new to *nix should try&error examples?)

For me, it's not matter to use sudo or not to use sudo with indication of privilege in doc. And I agree simple look of commands without sudo is nice.

So should I remove all sudo from docs related to kubeadm? Comments are very helpful.

@sftim
Copy link
Contributor

sftim commented Oct 24, 2019

Just a FYI, I don't have a lot of time to contribute to SIG Docs at the moment.
It sounds like other people are already taking part in a healthy discussion.

@neolit123
Copy link
Member

@vanou

So should I remove all sudo from docs related to kubeadm? Comments are very helpful.

if you are up for it, please go ahead.
but some sections would require notes about using root.
or just having a note on top of a document "some commands require root".

@vanou
Copy link
Contributor Author

vanou commented Oct 27, 2019

Hi All,
I have made issue (#17181) related to this. Please share your ideas/comments around this.

@tengqm
Copy link
Contributor

tengqm commented Oct 28, 2019

Adding 'sudo' wherever appropriate helps indicate the need of privilege and it hurts nobody.
/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 28, 2019
@k8s-ci-robot k8s-ci-robot merged commit 4fe0eaa into kubernetes:master Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants