-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Instance group wait for status #4783
Instance group wait for status #4783
Conversation
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=187192" |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=187380" |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccInstanceGroupManager_stateful|TestAccRegionInstanceGroupManager_distributionPolicy You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=187386" |
"wait_for_instances_status": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
Default: "STABLE", |
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.
I think we need a check on this value in Read- if it's ""
, set it to "STABLE"
. That way we get away without a state upgrader and don't have a one-off diff for resources provisioned with older provider versions.
When `STABLE` is specified this resource will wait until the instances are stable before returning. When `UPDATED` is | ||
set, it will wait for the version target to be reached and any per instance configs to be effective as well as all | ||
instances to be stable before returning. The possible values are `STABLE` and `UPDATED` | ||
|
||
--- | ||
|
||
* `auto_healing_policies` - (Optional) The autohealing policies for this managed instance |
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.
We want to document the attribute fields under status
too, right?
@@ -790,6 +790,7 @@ resource "google_compute_instance_group_manager" "manager" { | |||
// block on instances being ready so that when they get deleted, we don't try | |||
// to continue interacting with them in other resources | |||
wait_for_instances = true | |||
wait_for_instances_status = "UPDATED" |
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 changed due to a failing test, or just to ensure we use it somewhere? If it's just to use it, can we do so in an IGM-specific test?
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.
Also, can that test migrate from one to the other?
2d93ed1
to
f2fbbb4
Compare
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=188946" |
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.
LGTM- if you don't mind provisioning a MIG with an old version and then updating to the dev provider including this change locally, that will ensure that we handle the state transition correctly.
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=189109" |
Yup, wait_for_instances_status gets set to STABLE in state with no diff |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccInstanceGroupManager_waitForStatus You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=189203" |
Fixes: hashicorp/terraform-provider-google#8311
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)