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

Use /usr/sbin instead of /usr/local/bin to accommodate RHEL #300

Merged
merged 2 commits into from
Feb 9, 2022

Conversation

YrrepNoj
Copy link
Contributor

@YrrepNoj YrrepNoj commented Feb 8, 2022

Closes #296

@YrrepNoj YrrepNoj added the bug 🐞 Something isn't working label Feb 8, 2022
@YrrepNoj YrrepNoj self-assigned this Feb 8, 2022
@YrrepNoj
Copy link
Contributor Author

YrrepNoj commented Feb 8, 2022

/test all

@RothAndrew
Copy link
Contributor

According to this I'm concerned about putting stuff in /sbin. Is there a better directory we could pick? From reading that post, it sounds like /usr/sbin would be a better choice.

@jeff-mccoy
Copy link
Contributor

I don't think it matters much if you remember this is for appliance mode only, k3s will be a boot item as well. If /usr/sbin is always in the rhel path (note that link was for Ubuntu, which doesn't have this issue), then that's probably fine. I'm not certain that's the case but am fairly certain that /sbin or /bin always is regardless of the os. We'd probably want to verify running as root vs sudo too since this is really an env variable issue.

Alternatively, we could just add /usr/local/bin to the path as an env variable on exec instead.

@RothAndrew
Copy link
Contributor

I don't think it matters much if you remember this is for appliance mode only, k3s will be a boot item as well. If /usr/sbin is always in the rhel path (note that link was for Ubuntu, which doesn't have this issue), then that's probably fine. I'm not certain that's the case but am fairly certain that /sbin or /bin always is regardless of the os. We'd probably want to verify running as root vs sudo too since this is really an env variable issue.

Alternatively, we could just add /usr/local/bin to the path as an env variable on exec instead.

My assumption is that the rhel8 vagrant box that we have set up is representative of RHEL in general. Given that assumption, RHEL does include /usr/sbin by default in the PATH. Here's a screenshot:

image

Given this. I propose we go with /usr/sbin instead of /sbin. I do think it would work either way, but /usr/sbin feels more aligned with the stated intention of these different directories in Linux.

@jeff-mccoy
Copy link
Contributor

Works for me

@YrrepNoj YrrepNoj force-pushed the 296-move-binary-to-sbin branch from 2c844de to f39fe1d Compare February 8, 2022 20:59
@YrrepNoj YrrepNoj changed the title 296: Using /sbin instead of /usr/local/bin to accommodate RHEL 296: Using /usr/sbin instead of /usr/local/bin to accommodate RHEL Feb 8, 2022
@YrrepNoj
Copy link
Contributor Author

YrrepNoj commented Feb 8, 2022

Funnily enough (at least on the Vagrant rhel8) /sbin simply symbolically links to usr/sbin. Changed to usr/sbin.

@YrrepNoj YrrepNoj force-pushed the 296-move-binary-to-sbin branch from f39fe1d to af45273 Compare February 8, 2022 21:02
@YrrepNoj
Copy link
Contributor Author

YrrepNoj commented Feb 8, 2022

/test all

rm -f /usr/sbin/ctr
rm -f /usr/sbin/crictl
rm -f /usr/sbin/kubectl
rm -f /usr/sbin/k9s
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't know if it matters but this line shouldn't exist anymore as we don't install k9s separately now. Was missed during #237.

@jeff-mccoy
Copy link
Contributor

/test all

@jeff-mccoy jeff-mccoy enabled auto-merge (squash) February 9, 2022 02:43
@jeff-mccoy jeff-mccoy merged commit 7636dd0 into master Feb 9, 2022
@jeff-mccoy jeff-mccoy deleted the 296-move-binary-to-sbin branch February 9, 2022 02:57
@RothAndrew RothAndrew changed the title 296: Using /usr/sbin instead of /usr/local/bin to accommodate RHEL Use /usr/sbin instead of /usr/local/bin to accommodate RHEL Feb 9, 2022
Noxsios pushed a commit that referenced this pull request Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to inject seed image on RHEL host.
3 participants