search

scottwinkler / terraform-provider-shell

Terraform provider for executing shell commands and saving output to state file
Mozilla Public License 2.0
279 stars 60 forks source link

remove all ForceNew from resource.shell_script schema #61

Closed mrzor closed 4 years ago

mrzor commented 4 years ago

Fixes #59 I removed them all - none make sense to me in any context : if you're changing say, interpreter or triggers, you know what you're doing.

If the shell resource represents an actual resource (as opposed to a state-only construct), it seems to me that the most likely case is that no matter the change, you are still managing the same resource. You will therefore be able to refresh state either with terraform refresh (using read) or terraform apply (using update).

If the shell resource represents a state-only construct, delete is probably a no-op, and ForceNew never makes sense.

Given that the shell resource can represent anything, if the operator does need to destroy things before applying changes, terraform destroy can accommodate this rare need.

I'm happy to propose a fix limited to create/read if you find my argument unconvincing.

scottwinkler commented 4 years ago

Okay looks good for the most part. Except triggers needs to stay ForceNew. It's exactly the same as the triggers attribute on a null_resource, it forces the destruction and recreation of the resource.

Since you are making changes here, you should also make changes to the shell data source. Thanks.

mrzor commented 4 years ago

I addressed your concerns and force pushed a single commit for a cleaner merge.

I wasn't aware datasource schemas had a meaningful ForceNew. The relevant SDK documentation only speaks about resources and not data sources. Maybe I'm confused.

I noticed a glitch using master where environment was changed and the output wasn't marked as unknown, and the resource wasn't marked for re-creation (just for update) - despite ForceNew being in master's schema. Something is amiss on master - I couldn't dig further at the time and so haven't filed an issue.

Merging this patch could make diagnosing the glitch origin more complex. As long as it sits in a branch it can't be the culprit. I would refrain from merging this patch until I'm sure environment->output->deps behaves correctly.