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

Warn if running install-plotter.sh as root #16206

Merged
merged 2 commits into from
Jan 25, 2024

Conversation

ChiaMineJP
Copy link
Contributor

@ChiaMineJP ChiaMineJP commented Aug 31, 2023

Confirmed to work on

  • Fedora 35
  • Ubuntu 22.04
  • MacOS 13

@ChiaMineJP ChiaMineJP added the Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog label Aug 31, 2023
@ChiaMineJP ChiaMineJP requested a review from a team as a code owner August 31, 2023 12:06
@ChiaMineJP ChiaMineJP self-assigned this Aug 31, 2023
@ChiaMineJP ChiaMineJP changed the title No need to be root user to run install-plotter.sh No need to be root to run install-plotter.sh Aug 31, 2023
@arvidn
Copy link
Contributor

arvidn commented Aug 31, 2023

your change to the code appear to be the opposite of the title of the PR.

The check you're removing (appears to) make the script fail if you run it as root. This seems like a quite reasonable check.
Why do you think it should be removed?

@ChiaMineJP ChiaMineJP changed the title No need to be root to run install-plotter.sh Allow to be root to run install-plotter.sh Aug 31, 2023
@ChiaMineJP
Copy link
Contributor Author

ChiaMineJP commented Aug 31, 2023

@arvidn Sorry the title was wrong and I updated it.
I was asked if the non-root requirement is truly necessary.
There are some cases where users want to be root when running install-plotter.sh and I wanted to hear the comments from reviewers if there are background requirement which definitely needs to be a non-root user.

The specific case is that an user of chia-docker wanted to install bladebit on the docker image but chia-docker runs as root for some reasons.

@Starttoaster
Copy link
Contributor

your change to the code appear to be the opposite of the title of the PR.

The check you're removing (appears to) make the script fail if you run it as root. This seems like a quite reasonable check. Why do you think it should be removed?

Changes to chia-docker are being considered to allow the script to work as-is. However, that will be a breaking change to existing chia-docker users. This check in the install-plotter script causes it to exit if the user running the script is root. The script works (with this check removed) if the user is running as root.

Understanding that it's not ideal from a systems administration perspective to advise people to run as root. But, to me, the flip side of this is that it does come across as unnecessary prescription of ideals to explicitly require that the user be not-root for no reason other than general advisement. Should we play the systems administrator role for people running our scripts? That's a philosophical question maybe, I don't know the answer to it.

@arvidn
Copy link
Contributor

arvidn commented Sep 10, 2023

do you think it would make sense to just demote this to a warning message? But still allow it to start

@ChiaMineJP
Copy link
Contributor Author

do you think it would make sense to just demote this to a warning message? But still allow it to start

I think it's good.

@Starttoaster
Copy link
Contributor

do you think it would make sense to just demote this to a warning message? But still allow it to start

I'm fine with a warning message if that makes the change more palatable. I just don't personally see the point in forcing the script to exit, outside of an actual technical challenge running as non-root solves. Anyone running as root on a linux server understands, or should understand by themselves, why running as root is not ideal. I wouldn't generally consider it the job of the software I run to instill better sysadmin hygiene habits in me 😄

@ChiaMineJP ChiaMineJP force-pushed the cmj.install-plotter-no-root branch from 0ace805 to 7f342e6 Compare September 12, 2023 13:04
@ChiaMineJP
Copy link
Contributor Author

@arvidn @Starttoaster
I've updated to just warn and not exit.

@wallentx
Copy link
Contributor

wallentx commented Sep 23, 2023

Just a general rant about how we use sudo in some of our install scripts, and why I think it's a good idea to do less on behalf of the user, when it comes to our installers:

I think it might be somewhat bad etiquette to force root/sudo within scripts, especially when it comes to a sensitive environment.

I know for any other project I use, I tend to frown when I see that their installer uses sudo to execute several things that I might not particularly agree with (i.e. putting binaries in /sbin, or force installing packages).

If the user has the appropriate permissions, then they should be able to execute the script via sudo, if appropriate for their environment.

Elevated script execution should be the responsibility of the user, since the user should be ultimately play an involved role in executing scripts with elevated permissions, with an understanding of the consequences of doing so on their particular system.

As to completely disallowing this to be executed by root... I'm not sure why a user would want to do anything related to Chia as root, but I don't think that there's a technical reason that would prevent anything from working. I would predict that there are more things that could go wrong by doing this, but I think that's between the user, and their gods.

@wallentx wallentx changed the title Allow to be root to run install-plotter.sh Warn if running install-plotter.sh as root Sep 23, 2023
@Starttoaster Starttoaster merged commit 927ea40 into main Jan 25, 2024
@Starttoaster Starttoaster deleted the cmj.install-plotter-no-root branch January 25, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants