Add salt-masterless provisioner #14720
Conversation
|
I just finished adapting all of the Packer unit tests to Terraform. They should all be passing now. |
|
RFR? |
|
@jbardin Anything I can do to help get this merged? Writing more test cases etc.? |
|
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. |
|
Rebased again, build is green. |
|
Again, copied the doc from: https://www.packer.io/docs/provisioners/salt-masterless.html The example probably needs modification. |
|
Rebased again. |
|
Rebased again. |
| @@ -0,0 +1,468 @@ | |||
| // This package implements a provisioner for Terraform that executes a | |||
| // saltstack state within the remote machine | |||
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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.
|
Feedback pushed. |
There was a problem hiding this comment.
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.
|
🎉 |
|
Thanks a lot to all the people involved :-) |
|
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. |
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.