-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Move RIGM/IGM updating to use versions and update_policy #2546
Move RIGM/IGM updating to use versions and update_policy #2546
Conversation
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesNo diff detected in terraform-google-conversion. New Pull RequestsI built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
81c0d09
to
6289795
Compare
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple minor comments, but overall, looks awesome.
@@ -358,17 +361,16 @@ func resourceComputeRegionInstanceGroupManagerCreate(d *schema.ResourceData, met | |||
Name: d.Get("name").(string), | |||
Description: d.Get("description").(string), | |||
BaseInstanceName: d.Get("base_instance_name").(string), | |||
<% if version.nil? || version == 'ga' -%> | |||
<% if version == 'ga' -%> | |||
<% if version == 'ga' -%> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be duplicated here?
d.Set("version" , nil) | ||
|
||
<% if version == 'ga' -%> | ||
// rolling_update_policy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment doesn't really tell me anything.
Hi! I'm the modular magician, I work on Magic Modules. Pull request statusesterraform-provider-google-beta already has an open PR. New Pull RequestsI didn't open any new pull requests because of this PR. |
For fields like instance_template there should be a clean path to move from the old way of specifying to the new way of specifying without a broken config or casuing an apply.
Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.
6444ada
to
e0e5b6e
Compare
Moving the beta functionality for multiple version control from beta to GA. Also deprecating the old way of specifying the instance template and update strategies to be removed in 3.0.0
I ended up marking some required fields as Optional/Computed in order to make sure that there is a seamless transition from using
instance_template
the old way to the new way. My intent is to markversion
and some of those other fields as required again with the 3.0.0 upgrade when I remove the old way of specifying them.Pre-work for: hashicorp/terraform-provider-google#4555
Release Note Template for Downstream PRs (will be copied)