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

Update Salt provisioner to allow more flexibility in passing arguments to the bootstrap script #5435

Merged
merged 3 commits into from
Jul 9, 2015

Conversation

timoguin
Copy link

@timoguin timoguin commented Mar 4, 2015

I needed the ability to specify a custom repository to install Salt from (our forked version), but the provisioner currently formulates the arguments based on the install_type option.

This PR adds a new install_type called custom so that the arguments aren't formatted.

This let's me do something like this if I need to test a patched version of Salt:

salt.install_args = "-g https://github.com/juiceinc/salt.git git bugfix/es-returner"

Those options will get passed as-is to the bootstrap script, so any option that's available upstream in that script should work.

…re flexibility in passing arguments to the bootstrap script. Updated the docs.
@timoguin
Copy link
Author

@cachedout, just tagging you in this since I think you are the official maintainer for the Salt provisioner.

@cachedout
Copy link

@timoguin I'm afraid I don't have commit privileges on this repo, if that's what you were asking. I'm happy to maintain it but @mitchellh or somebody on the core team will have to make that decision. :]

As far as the PR goes, looks good to me.

@timoguin
Copy link
Author

@cachedout I know you don't have write privileges. Just thought you should be IN THE KNOW (TM). :)

@cachedout
Copy link

Ah, OK. Cool. I appreciate the ping!

@sethvargo
Copy link
Contributor

HI @timoguin

Thank you for the Pull Request, and I apologize for the delay in reviewing. After some careful thought (and going through a similar thing with the Chef provisioner), I think we should just add a bootstrap_command or install_command attribute that lets the user fully customize the installation experience.

I think this would solve all the problems you have identified. I am not a fan of a "custom" install type because that is ambiguous. If the user chooses to specify the install_command, he/she will also need to specify all the required flags, but I think that is appropriate since this is a power-user case. What do you think?

Also, this will no longer merge cleanly. If you agree with the install_command approach, could you please do a rebase?

@timoguin
Copy link
Author

timoguin commented Jun 1, 2015

@sethvargo I think that makes much more sense. I've rebased my branch and will be pushing your suggested modifications shortly.

@timoguin
Copy link
Author

timoguin commented Jun 1, 2015

@sethvargo Modifications made. I accidentally did a normal merge instead of a rebase when pulling upstream. Let me know if that's a problem. I can delete this PR and resubmit if necessary.

@mitchellh
Copy link
Contributor

LGTM

mitchellh added a commit that referenced this pull request Jul 9, 2015
Update Salt provisioner to allow more flexibility in passing arguments to the bootstrap script
@mitchellh mitchellh merged commit d51c5fb into hashicorp:master Jul 9, 2015
@nmadhok
Copy link

nmadhok commented Jul 16, 2015

This merge breaks the bootstrap process since if salt.install_command is not specified, it adds Vagrant::Config::V2::DummyConfig to the bootstrap_options which breaks everything. Can this be reverted and back ported into vagrant 1.7.3?

@mpolden
Copy link

mpolden commented Jul 20, 2015

@nmadhok Sounds like this change might have caused #6011 too?

@ghost ghost locked and limited conversation to collaborators Apr 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants