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

Do not have bootstrap run sudo commands. #3113

Merged
merged 1 commit into from
Jul 16, 2020
Merged

Do not have bootstrap run sudo commands. #3113

merged 1 commit into from
Jul 16, 2020

Conversation

dracos
Copy link
Member

@dracos dracos commented Jul 13, 2020

Your site user may not have access to sudo, and your admin user may cause permission issues if used to do the git checkout, or the Perl module installation. Document the separate script to install system packages instead.

Fixes #2930. I think it causes too much confusion to have it run sudo-level commands, when it's a user-level script. I realise this was introduced because a new Perl module needed a new library package, I've added some documentation about updating.

I've left in the Docker fms user being able to run bin/install_packages, don't see why not, and have changed the preinit to run this as well. I'm not sure if that's everything.

@dracos dracos requested a review from sagepe July 13, 2020 13:04
@codecov
Copy link

codecov bot commented Jul 13, 2020

Codecov Report

Merging #3113 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3113   +/-   ##
=======================================
  Coverage   83.42%   83.42%           
=======================================
  Files         248      248           
  Lines       15574    15574           
  Branches     2906     2906           
=======================================
  Hits        12992    12992           
  Misses       1660     1660           
  Partials      922      922           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67823bc...1243e70. Read the comment docs.

Copy link
Member

@sagepe sagepe left a comment

Choose a reason for hiding this comment

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

Looks fine, but I think it may be worth adding a call to install_packages in the Vagrantfile where the mySociety box is used - this way we'll ensure that new packages are installed before Perl modules are updated, ensuring any new build dependencies are met. I'll add a commit - see what you think.

@dracos
Copy link
Member Author

dracos commented Jul 15, 2020

Sounds good. Have squashed together; do you want to do a check of all this and then we can do a 3.0.2 release perhaps?

bin/docker.preinit Outdated Show resolved Hide resolved
Your site user may not have access to sudo, and your admin user may
cause permission issues if used to do the git checkout, or the Perl
module installation. Document the separate script to install system
packages instead.

Co-authored-by: Sam Pearson <sam@mysociety.org>
Copy link
Member

@sagepe sagepe left a comment

Choose a reason for hiding this comment

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

:shipit:

@dracos dracos merged commit 8a37b57 into master Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

script/update no longer working for some installations
2 participants