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

Don't set metadata_startup_script in some cases. #1081

Merged
merged 4 commits into from
Feb 14, 2018

Conversation

nat-henderson
Copy link
Contributor

If the state contains metadata.startup-script, the user likely wants to keep it in there instead of metadata_startup_script (which is ForceNew, where metadata.startup-script is not). This isn't the standard case - metadata.startup-script creates heterogeneous fleets unless managed very carefully, which is why metadata_startup_script exists - but it's supported by the API and it's a valid state, so we want it to continue working.

After this PR:

  • If you set metadata_startup_script and don't set metadata.startup-script, you will see no change.
  • If you set metadata.startup-script and don't set metadata_startup_script, you will stop seeing permanent diffs, because Read will stop setting metadata_startup_script.
  • If you set both, your behavior will be strange and you'll probably see permanent diffs - don't set both.
  • If you do an import, you'll get metadata_startup_script.

If you're affected by something like #1079, this will make sure that you don't have problems going for anything created after this PR goes in, but for your existing instances you'll need to make sure that the metadata.startup-script has a non-empty value, probably using the terraform state functions.

@rosbo
Copy link
Contributor

rosbo commented Feb 13, 2018

If you set both, your behavior will be strange and you'll probably see permanent diffs - don't set both.

Should we add a ConflictsWith to ensure people don't set both?

@nat-henderson
Copy link
Contributor Author

Can't, unfortunately - metadata is a map. Probably need to put it in the validator - I'll take a look at that. If it's easy I'll add it to this PR.

@nat-henderson
Copy link
Contributor Author

Shoot - I've got a problem with updates from metadata.startup-script -> metadata_startup_script. I'll try and fix it...

@nat-henderson
Copy link
Contributor Author

Got it. Running the tests.

@nat-henderson
Copy link
Contributor Author

All the tests passed, merging.

@nat-henderson nat-henderson merged commit f785116 into hashicorp:master Feb 14, 2018
@nat-henderson nat-henderson deleted the metadata-startup branch February 14, 2018 07:55
ishashchuk pushed a commit to wayfair-archive/terraform-provider-google that referenced this pull request Feb 20, 2018
modular-magician pushed a commit to modular-magician/terraform-provider-google that referenced this pull request Oct 7, 2019
@ghost
Copy link

ghost commented Mar 29, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

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

Successfully merging this pull request may close these issues.

2 participants