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
Be more liberal in our license acceptance checking. #6574
Conversation
Now if you're running hab commands as a non-privileged user and a privileged user has already accepted the license, it won't prompt you twice. Signed-off-by: Josh Black <raskchanky@gmail.com>
Hello raskchanky! Thanks for the pull request! Here is what will happen next:
Thank you for contributing! |
fn superuser_license_root() -> PathBuf { PathBuf::from(&*FS_ROOT_PATH).join("hab") } | ||
|
||
fn user_license_root() -> PathBuf { | ||
if let Some(home) = dirs::home_dir() { | ||
home.join(".hab") | ||
} else { | ||
panic!("No home directory available. Unable to find a suitable place to write a license \ |
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.
Just a question on whether panic is what we want here.. I realize its code thats not specifically for this change but it seems odd to have a panic in that path
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.
Yeah the panic is intentional. If we hit that code path, something is super wrong because it means that hab
is being run by a user that doesn't have a home directory. Users without home directories tend to be ones that are reserved for running services and as such, wouldn't be appropriate for accepting a license, since there's no sensible place to persist the license acceptance file.
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.
LGTM
Signed-off-by: Josh Black <raskchanky@gmail.com>
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.
components/hab/src/license.rs
Outdated
pub fn license_exists() -> bool { license_file().is_file() } | ||
pub fn license_exists() -> bool { | ||
license_file(&license_path(&superuser_license_root())).is_file() | ||
|| license_file(&license_path(&user_license_root())).is_file() |
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.
Though I doubt it ends up making a huge difference, it seems like we should check the user license first, then the superuser license, since that's going from more specific to more general.
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.
Sure, why not
Signed-off-by: Josh Black <raskchanky@gmail.com>
Obvious fix; these changes are the result of automation not creative thinking.
Now if you're running hab commands as a non-privileged user and
a privileged user has already accepted the license, it won't prompt you
twice.
Signed-off-by: Josh Black raskchanky@gmail.com