Ensure state and pillar remote locations are always defined

[jcockhren]

Uses the defined constants for remote pillar and state tree locations as variable defaults. Then compares the retrieved variable values against those defaults.

Fixes #17150

[jcockhren]

@apparentlymart Hey. Is there any way I can enhance this PR so it can be included in the next release?

[apparentlymart]

Hi @jcockhren! Sorry we missed this before.

The Terraform team at HashiCorp doesn't have any in-house expertise on Salt in order to understand the implications of this and test it. 😖

@sevagh, do you have any thoughts on this and would you be able to try it out? I ask just because you originally contributed this provisioner so I'm hoping you have a suitable environment available for testing.

[sevagh]

Unfortunately I use neither Terraform nor Salt these days. Some kinda Docker-based test harness might be a thought. Or possibly deprecating the provisioner entirely.

[jcockhren]

@apparentlymart is developing something like @sevagh is describing something the Terraform team would be willing to pair with me on? What are you all using currently to test all the supported provisioners outside of the resource_provisioner_test test files?

[apparentlymart]

Hi @jcockhren,

Aside from what you see in the repository there are no ongoing tests for the provisioners. For those provisioners that depend on external software they are usually tested manually whenever they are changed. So far they have changed infrequently enough that it hasn't been worth the opportunity cost to implement something more sophisticated, and there's generally been a community maintainer for each provisioner. We are open to introducing more complex automated tests (this has been done in a few providers, for example, with test harness scripts to spin up dependencies in Docker).

Unfortunately we don't have a lot of spare cycles right now since we're heads down on a big project to improve the configuration language parser and interpreter. I'm sorry we can't work closely on a project in this area right now, even though I'd really like to have a more robust answer to running tests for these provisioners. 😖

[jcockhren]

@apparentlymart That's fine. I thought I'd at least ask. :)

So does this mean the further improvements on this provisioner is blocked until further notice? That seems drastic and unlike how things were done before. Could we get this merged if there's other community members that can verify this PR with a workflow similar to what was done in #16704?

@subbarao , @gapotts or @marco-m could either of you assist in verifying this fix addresses issue #17150?

[subbarao]

@jcockhren fix looks good to me

[apparentlymart]

Hi @jcockhren!

Ideally we'd prefer to find a new maintainer for this provisioner who has a suitable environment to test it and has enough Salt expertise to be able to properly review PRs. However, in the absence of that I would consider merging it if we could find a few users who are willing to try this out and see if it works in their environment.

If possible it'd be great to have some tests for this functionality in resource_provisioner_test.go. I'm not sure how feasible that is since the tests in this repo ought to be runnable without any special additional software installed, but there are some existing tests in there which might show a reasonable pattern for testing this.

For what it's worth, I'm sorry that this provisioner has landed in such a strange situation. Last year we reached the point where there were enough providers that it was no longer feasible to maintain them all as a part of Terraform Core, and we may have now reached that point with provisioners. For the short term I'd like to see it we can compromise by having community maintainers of the provisioners within this repository, but when we get a chance we'll also look at what it might take to split the provisioners out as was done for providers so that they can each have their own release lifecycle and the Terraform Core development team won't be a review/release bottleneck.

[adamehirsch]

Having just tripped over this bug this morning, I'd be thrilled to try out this PR, which looks pretty reasonable to me. How could I go about testing it locally?

[jcockhren]

@adamehirsch yes please! I've been using a custom build based on this PR for months. Let me know how your testing goes.

[jcockhren]

@adamehirsch I just realized I may have not answered your question fully:

1. Follow the directions here to make a custom build of terraform: https://github.com/hashicorp/terraform#developing-terraform
2. Feel free to use the terraform configuration outlined in the original issue Salt Pillar and State roots not moved to final location #17150 as a starting point.
3. Any functional/minimal pillar, state and minion config would work.

[adamehirsch]

@jcockhren I compiled with your patch last night, tested it this morning and it worked perfectly! I would totally concur that this can/should be merged to master, @apparentlymart

[adelcast]

I bumped into this issue this morning, went over the code, applied @jcockhren patch and I am up and running. Since this is change is a) very small, b) clearly fixing something broken, I would like to add my voice to the choir: we are all better off if this gets pulled in, instead of waiting to see if a new maintainer steps up.

[jcockhren]

but when we get a chance we'll also look at what it might take to split the provisioners out as was done for providers so that they can each have their own release lifecycle and the Terraform Core development team won't be a review/release bottleneck.

@apparentlymart Has the Terraform Core development team reached this point yet?

[R0quef0rt]

Why hasn't this been merged yet? Clearly, it fixes a problem that affects ALL users of the salt-masterless provisioner. The provisioner is useless in its current form.

This PR contains just 5 lines of code. You don't need to be an Salt expert to see that it isn't going to break anything. I'm using it myself; and it works great. Can we just merge and be done with it?

[SpencerSharkey]

Bump. Would love to see this land for the next release. Having to deploy our own build of terraform is cumbersome, especially for such a small goof.

[jcockhren]

bump

[SEAPUNK]

What are the remaining blockers to merge this PR? We'd love to see this change land, so I'm willing to help if there's work remaining before actually merging.

[exarkun]

I am also trying to use this provisioner and immediately tripped over the problem that the documentation is wrong and provisioning fails because there is not default remote state or pillar location.

[cwage]

Any reason this hasn't been merged yet?

[apparentlymart]

Thanks for testing this, everyone. I'm going to merge this now, for inclusion in the forthcoming v0.12.0 release.

[jcockhren]

@apparentlymart One more question: In order to be included in the CHANGELOG for v0.12.0, is there a contributor agreement I must sign or anything?

[ghost]

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.