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

Wrap shell scripts for safety #155

Merged
merged 1 commit into from Nov 9, 2015
Merged

Conversation

@aneeshusa
Copy link
Member

aneeshusa commented Nov 5, 2015

Wrap the entirety of the install_salt and configure_salt shell scripts
in a function to prevent problems if interrupted mid-download, as
preparation to update the installation guide in the wiki to use these
scripts via curl | sudo sh.

Also make scripts portable and do a bit of cleanup.

Review on Reviewable

Wrap the entirety of the install_salt and configure_salt shell scripts
in a function to prevent problems if interrupted mid-download, as
preparation to update the installation guide in the wiki to use these
scripts via curl| sudo sh.

Also make scripts portable and do a bit of cleanup.
@aneeshusa
Copy link
Member Author

aneeshusa commented Nov 5, 2015

The install instructions on the wiki don't pin the salt version, which will cause problems if run afresh, so this will allow reusing the install script from the repo for the install instructions to keep the versions in sync. (Right now, installing salt from homebrew on OS X will break everything as 2015.8.0 is bad, and 2015.8.1 isn't packaged in homebrew yet.)

@metajack
Copy link
Contributor

metajack commented Nov 8, 2015

Question below, otherwise looks great.


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


.travis/configure_salt, line 5 [r1] (raw file):
Is pipefail not needed anymore?


.travis/install_salt, line 5 [r1] (raw file):
Ditto


Comments from the review on Reviewable.io

@aneeshusa
Copy link
Member Author

aneeshusa commented Nov 8, 2015

Unfortunately, raw sh doesn't support set -o pipefail, only bash and zsh do.

@metajack
Copy link
Contributor

metajack commented Nov 9, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Nov 9, 2015

📌 Commit fbc7250 has been approved by metajack

@bors-servo
Copy link
Contributor

bors-servo commented Nov 9, 2015

Testing commit fbc7250 with merge 60578a6...

bors-servo added a commit that referenced this pull request Nov 9, 2015
Wrap shell scripts for safety

Wrap the entirety of the install_salt and configure_salt shell scripts
in a function to prevent problems if interrupted mid-download, as
preparation to update the installation guide in the wiki to use these
scripts via curl | sudo sh.

Also make scripts portable and do a bit of cleanup.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/155)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 9, 2015

☀️ Test successful - travis

@bors-servo bors-servo merged commit fbc7250 into servo:master Nov 9, 2015
1 of 3 checks passed
1 of 3 checks passed
code-review/reviewable Review in progress: all files reviewed, 2 unresolved discussions
Details
homu Testing commit fbc7250 with merge 60578a6...
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@aneeshusa
Copy link
Member Author

aneeshusa commented Nov 9, 2015

Thanks for getting this merged! I updated the Linux installation instructions on the wiki, but I don't have a Mac and am not confident about updating the current Mac instructions to use the install script as well. Can I get some help with that, or can someone with more Mac experience update those instructions to be more in line with the Linux ones?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.