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

Minimized a chance where sudo prompts users for password #13219

Merged
merged 9 commits into from
Oct 4, 2022

Conversation

ChiaMineJP
Copy link
Contributor

If python, sqlite, openssl are already available, we don't need to run sudo apt install xxx.
This PR tries to minimize the chance to prompt users for their password.

@ChiaMineJP ChiaMineJP requested a review from a team as a code owner August 29, 2022 15:03
@paninaro paninaro requested a review from altendky August 29, 2022 22:19
@ChiaMineJP ChiaMineJP self-assigned this Aug 30, 2022
Copy link
Contributor

@altendky altendky left a comment

Choose a reason for hiding this comment

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

The openssl bit was specifically intended to trigger an update, not just an install when missing. It was added when there was a critical ssl update a few months ago. I'm not sure if that is still considered important.

Thanks for the work on this!

install.sh Show resolved Hide resolved
install.sh Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
install.sh Outdated Show resolved Hide resolved
@hoffmang9
Copy link
Member

As python just uses the system installed OpenSSL we want to be forcing an upgrade to the most current OpenSSL on an ongoing basis. This will certainly not be the last time OpenSSL has a vulnerability...

@altendky
Copy link
Contributor

I had initially proposed a CLI flag to skip the system stuff. I don't want to be entering my password into scripts all the time. I also don't want to wait around for apt all the time. A CLI flag would leave the option for default users to still get the baby sitting we have here. Someone recently got banned from apt connections or something and so they couldn't use the installer for a few hours. That's kind of the top level perspective that brought up this topic. I wonder if the full suite of mature installers at this time has any bearing on the trade offs here.

@ChiaMineJP
Copy link
Contributor Author

@hoffmang9 @altendky
Thank you for the comment.
I'll change the PR to:

  • Add a cli flag to set PACKAGE_INSTALL_REQUIRED forcibly to 0
  • Set PACKAGE_INSTALL_REQUIRED=1 when OpenSSL version is old if the flag above is not specified.

@altendky
Copy link
Contributor

altendky commented Sep 1, 2022

I think to check if openssl is old I would basically expect to do an apt update and then ask it. At that point we maybe may as well just do the install instead? Unless you can ask apt if there are new versions without doing the global/sudo-requiring apt update?

@ChiaMineJP
Copy link
Contributor Author

OpenSSL will be updated only when version < 3.0.2 or 1.1.1n or 1.0.2zd is installed.
We won't update openssl just because there are new versions.

@altendky
Copy link
Contributor

altendky commented Sep 1, 2022

I thought Gene wanted us to do that. Otherwise we have to push an updated install.sh to everyone to get the benefit of this when there's another important update to openssl. I'm on the fence about it being our place to update the user's system, but the upside is certainly pretty straightforward.

@ChiaMineJP
Copy link
Contributor Author

Overhauled entire construct of install.sh.
Added -s option. (Skip python install)
If you don't specify -s, then install.sh works just the same as usual.
If you specify -s, Python installation (as well as openssl/sqlite) will be skipped.

Copy link
Contributor

@emlowe emlowe left a comment

Choose a reason for hiding this comment

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

I like this idea of a skip option

@ChiaMineJP ChiaMineJP added the ready_to_merge Submitter and reviewers think this is ready label Oct 4, 2022
install.sh Show resolved Hide resolved
@altendky altendky dismissed their stale review October 4, 2022 14:02

raised concerns have been addressed

@wallentx wallentx merged commit 9fa965b into main Oct 4, 2022
@wallentx wallentx deleted the cmj.we-do-not-need-sudo-apt-install-everytime branch October 4, 2022 16:19
@wallentx wallentx added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants