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

Add salt-masterless provisioner #14720

Merged
merged 3 commits into from
Aug 16, 2017
Merged

Add salt-masterless provisioner #14720

merged 3 commits into from
Aug 16, 2017

Conversation

sevagh
Copy link
Contributor

@sevagh sevagh commented May 21, 2017

This is practically copy-pasted from Packer - not sure how I should attribute the original authors. I just had to modify the Packer pieces to fit into the Terraform provisioner model (inspired by the existing chef provisioner).

Also, I need to write proper unit tests. This is a premature PR to get some eyeballs on it.

@sevagh
Copy link
Contributor Author

sevagh commented May 22, 2017

I just finished adapting all of the Packer unit tests to Terraform. They should all be passing now.

@sevagh
Copy link
Contributor Author

sevagh commented May 24, 2017

RFR?

@sevagh
Copy link
Contributor Author

sevagh commented Jun 5, 2017

@jbardin Anything I can do to help get this merged? Writing more test cases etc.?

@jbardin
Copy link
Member

jbardin commented Jun 6, 2017

Hi @sevagh,

Sorry about the delay. We're in the midst of organizing for the 0.10 release, which involves moving around a lot of the code related to providers and provisioners.

Once we get the repo in a stable state moving towards the next major release, we can definitely take a look at this PR.

@sevagh
Copy link
Contributor Author

sevagh commented Jun 22, 2017

Rebased again, build is green.

@sevagh
Copy link
Contributor Author

sevagh commented Jun 23, 2017

Again, copied the doc from: https://www.packer.io/docs/provisioners/salt-masterless.html

The example probably needs modification.

@sevagh
Copy link
Contributor Author

sevagh commented Jul 27, 2017

Rebased again.

@jbardin jbardin self-assigned this Aug 4, 2017
@sevagh
Copy link
Contributor Author

sevagh commented Aug 7, 2017

Rebased again.

Copy link
Member

@jbardin jbardin left a comment

Choose a reason for hiding this comment

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

Thanks putting in the work to port this over, and for your patience getting this reviewed! It looks good, and I just have a couple minor changes inline.

@@ -0,0 +1,468 @@
// This package implements a provisioner for Terraform that executes a
// saltstack state within the remote machine
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a doc comment here to reference the packer code for context and history, as well as attribution.

//
// Adapted from gitub.com/hashicorp/packer/provisioner/salt-masterless

return nil
}

func (p *provisioner) Cancel() {
Copy link
Member

@jbardin jbardin Aug 7, 2017

Choose a reason for hiding this comment

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

This can be removed, as it doesn't apply to the terraform provisioner.

we can work later on making this "stoppable", using comm.Disconnect so the provisioner exits in a controlled manner when Stop() is called on the provisioner.

@sevagh
Copy link
Contributor Author

sevagh commented Aug 7, 2017

Feedback pushed.

Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

This looks good to me.

We've been a little nervous about adding more "complex" provisioners in the vein of chef, rather than the simpler integration points like remote-exec and file, because the provisioner mechanism in Terraform is not as fleshed out as other concepts (e.g. the lifecycle for provisioners is pretty weak around cleaning things up when destroying, the results of them can't easily be used elsewhere, and they can't inherit configuration from a provider, etc).

However, we've had the chef provisioner in here long enough that we've learned some of the sour points of this sort of provisioner (such as what I described above) and it doesn't seem like this provider will trip over too much of that, given that this is "masterless" and thus the state lives entirely on the remote machine.

So including this sounds good to me, with the caveat that we'll remain cautious about adding more provisioners like this in the short term, until we're able to spend the time to do a more holistic investigation into the provisioner feature and address some of its limitations. If someone considering making a provisioner for another similar system finds this PR, I'd request that they open an issue first to discuss the tradeoffs so we can avoid wasted work.

But hypothetical future PRs aside, I'd just like to give a little time for the 0.10 dust to settle (in case a quick, bug-fixes-only patch release is needed) and then I think we can get this in.

@sevagh
Copy link
Contributor Author

sevagh commented Aug 7, 2017

🎉

@marco-m
Copy link
Contributor

marco-m commented Sep 30, 2017

Thanks a lot to all the people involved :-)

@ghost
Copy link

ghost commented Apr 7, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@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.

5 participants