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

Make Salt install/configure Vagrant 1.8 compatible #180

Merged

Conversation

@aneeshusa
Copy link
Member

aneeshusa commented Dec 22, 2015

The original Vagrantfile was meant to be used with Vagrant 1.7.4, but
required 2 patches to Vagrant for proper functionality, which I had
submitted as pull requests. Vagrant has since released a new version,
1.8.0, which has updates the Salt provisioner. PR status:

The second PR was rejected "in favor" of a third PR which simply
removed the config_dir option entirely instead of using sudo to be able
to write in a privileged directory. An unrelated PR removed a second
option we had been using: install_command.

These changes in Vagrant were due to incompatibilities with the official
Salt bootstrap script, so this commit brings the install_salt script's
behavior closer to that of the official Salt bootstrap script as well.
Namely, it can now copy over configuration files as part of
installation, using the same flags as the official script.

The configure_salt script is now used just to setup the salt and pillar
roots on Travis (this is accomplished via shared folders in Vagrant),
hence the renaming.

With this commit and the new Vagrant version, the included Vagrantfile
now works with a vanilla Vagrant 1.8.0 install, no patching needed.

Note to self: update the wiki when this is merged!

Review on Reviewable

The original Vagrantfile was meant to be used with Vagrant 1.7.4, but
required 2 patches to Vagrant for proper functionality, which I had
submitted as pull requests. Vagrant has since released a new version,
1.8.0, which has updates the Salt provisioner. PR status:
 - [One PR](hashicorp/vagrant#6474) was merged
 - [Another PR](hashicorp/vagrant#6473) was not

The second PR was rejected "in favor" of [a third
PR](hashicorp/vagrant#6073) which simply removed
the config_dir option entirely instead of using sudo to be able to write
in a priveleged directory. [An unrelated
PR](https://github.com/mitchellh/vagrant/pull/6382/files) removed a
second option we had been using: install_command.

These changes in Vagrant were due to incompabilities with the official
Salt bootstrap script, so this commit brings the install_salt script's
behavior closer to that of the official Salt bootstrap script as well.
Namely, it can now copy over configuration files as part of
installation, using the same flags as the official script.

The configure_salt script is now used just to setup the salt and pillar
roots on Travis (this is accomplished via shared folders in Vagrant),
hence the renaming.

With this commit and the new Vagrant version, the included Vagrantfile
now works with a vanilla Vagrant 1.8.0 install, no patching needed.
@aneeshusa aneeshusa force-pushed the aneeshusa:update-for-vagrant-1.8-compatibility branch from bfa9f49 to 49faf66 Dec 22, 2015
@aneeshusa
Copy link
Member Author

aneeshusa commented Dec 22, 2015

This is fairly low-importance, but help from someone with a Mac would be appreciated on this one! I have no idea how OS X got '.travis/install_salt' for $OS_NAME (via $1) in the error message...

@johntron
Copy link

johntron commented Jan 7, 2016

The title is misleading - I thought you were trying to undo the changes we waited so long to get. This is great though, thanks!

The build seems to be failing, because you've changed the parameter order of the call to install_salt, but the install_salt script wasn't updated to use the correct parameter index (maybe $4 instead of $1). This means that instead of receiving the value of ${TRAVIS_OS_NAME}, instead it gets something that's probably considered an invalid value - this may be be what's causing the error message. I could be wrong, but might be able to test by using the same call to install_salt as Travis:

.travis/install_salt -F -c .travis -- "osx"` # kill the command before it does anything
@aneeshusa
Copy link
Member Author

aneeshusa commented Jan 7, 2016

@johntron Sorry, I didn't realize the title was misleading. Feel free to change it to something that's more understandable.

Looking at your comments now.

@aneeshusa
Copy link
Member Author

aneeshusa commented Jan 7, 2016

Sorry, I should have mentioned this before - it looks like the problem is that getopt behaves differently on OS X vs. Linux. (The builds are successful on Linux hosts but not OS X hosts). I couldn't figure out how it differs from the man page, so I'd appreciate help from someone with a Mac that can investigate what getopt is doing.

I changed the parameter order to mirror the flags used by the official script, and am using getopt to parse them portably. Here's the behavior I want, which is what's happening on Linux: the arg-parsing loop should call shift after each arg it handles to bring the next one up to the first spot in "$1", and stop after it sees the "--". At this point, the only arg left should be the positional parameter (either "linux" or "osx"), which is why I'm reading "$1" into OS_NAME.

@aneeshusa
Copy link
Member Author

aneeshusa commented Jan 7, 2016

What's especially confusing to me is that ".travis/install_salt" should be $0, so I have no idea how it got into the $1 slot to go into OS_NAME in the error message from the Mac builds on Travis.

OS X does not use GNU getopt but rather BSD getopt, which is less
friendly. Update getopt usage and argument parsing to be portable.
@aneeshusa aneeshusa force-pushed the aneeshusa:update-for-vagrant-1.8-compatibility branch from 49faf66 to 36b881e Feb 5, 2016
@aneeshusa
Copy link
Member Author

aneeshusa commented Feb 5, 2016

I figured out how to make this work on OS X! It would be super helpful for me if this and #181 got merged in (ideally this one first).

@aneeshusa
Copy link
Member Author

aneeshusa commented Feb 5, 2016

Also, a note to myself: when this gets merged, remember to update the installation notes on the wiki.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Feb 8, 2016

1 similar comment
@larsbergstrom
Copy link
Contributor

larsbergstrom commented Feb 9, 2016

@jdm
Copy link
Member

jdm commented Feb 9, 2016

@bors-servo: r=larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Feb 9, 2016

📌 Commit 36b881e has been approved by larsbergstrom

@bors-servo
Copy link
Contributor

bors-servo commented Feb 9, 2016

Testing commit 36b881e with merge 9477824...

bors-servo added a commit that referenced this pull request Feb 9, 2016
…r=larsbergstrom

Make Salt install/configure Vagrant 1.8 compatible

The original Vagrantfile was meant to be used with Vagrant 1.7.4, but
required 2 patches to Vagrant for proper functionality, which I had
submitted as pull requests. Vagrant has since released a new version,
1.8.0, which has updates the Salt provisioner. PR status:
 - [One PR](hashicorp/vagrant#6474) was merged
 - [Another PR](hashicorp/vagrant#6473) was not

The second PR was rejected "in favor" of [a third PR](hashicorp/vagrant#6073) which simply
removed the config_dir option entirely instead of using sudo to be able
to write in a privileged directory. [An unrelated PR](https://github.com/mitchellh/vagrant/pull/6382/files) removed a second
option we had been using: install_command.

These changes in Vagrant were due to incompatibilities with the official
Salt bootstrap script, so this commit brings the install_salt script's
behavior closer to that of the official Salt bootstrap script as well.
Namely, it can now copy over configuration files as part of
installation, using the same flags as the official script.

The configure_salt script is now used just to setup the salt and pillar
roots on Travis (this is accomplished via shared folders in Vagrant),
hence the renaming.

With this commit and the new Vagrant version, the included Vagrantfile
now works with a vanilla Vagrant 1.8.0 install, no patching needed.

Note to self: update the wiki when this is merged!

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

bors-servo commented Feb 9, 2016

☀️ Test successful - travis

@bors-servo bors-servo merged commit 36b881e into servo:master Feb 9, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@aneeshusa
Copy link
Member Author

aneeshusa commented Feb 9, 2016

Thanks for getting this merged! Being able to use Vagrant locally is really useful in testing out changes (and prevents a ton of amended commits over small typos caught by Travis 😄). I've updated the Vagrant instructions on the wiki; it turns out the installation instructions don't need changing after all.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Feb 9, 2016

Thank you for all your help! Sorry this one lingered a bit in the review queue.

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

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