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

Add Beta support for auto_healing_policies to instance group manager. #249

Merged
merged 7 commits into from
Aug 2, 2017

Conversation

rileykarson
Copy link
Collaborator

Add support for the first Beta feature, auto_healing_policies, to google_compute_instance_group_manager. Beta support was added to the resource in #234.

Part of #93.
Fixes #48.

Beta features are documented by adding a third divider to the list of fields; in addition to Required, and Optional, there is now a Beta section, where Beta fields are listed.

When users begin using Beta fields, their resources will automatically "upgrade" to Beta, and begin performing operations using Beta APIs instead of v1. If they remove the Beta fields from their resource, it will then automatically "upgrade" back to v1.

By inferring the version from the fields a user uses, we;

  • Don't need to perform validation that we match different schema versions in CRUD methods.
  • Preserve existing Terraform state files.
  • Allow seamless "upgrades" back to v1 as features enter GA.

We do not currently have a means of explicitly notifying users what version an operation is being performed at; I'd like to investigate means of how to show the version to users and especially whether we can surface that information at the plan phase before committing to an implementation.

I don't believe that it should be blocking for this change, as both our documentation and the GCP documentation will communicate that a feature is in Beta to users.

@catsby

@@ -704,6 +799,121 @@ func testAccInstanceGroupManager_separateRegions(igm1, igm2 string) string {
`, igm1, igm2)
}

func testAccInstanceGroupManager_autoHealingPolicies(template, target, igm, hck string) string {
return fmt.Sprintf(`
resource "google_compute_instance_template" "igm-basic" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you align this to the left?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@danawillow
Copy link
Contributor

Thanks! I'm fine with this once @catsby is.

@@ -81,12 +81,24 @@ The following arguments are supported:
instances in the group are added. Updating the target pools attribute does
not affect existing instances.

---

* `auto_healing_policies` - (Optional, Beta) The autohealing policies for this managed instance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra spaces between (Optional, Beta) and The

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


// This test is to make sure that a single version resource can link to a versioned resource
// without perpetual diffs because the self links mismatch.
// Once auto_healing_policies is no longer beta, we will need to use a new field or resource.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a new field or resource for what? Not super clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Steps: []resource.TestStep{
resource.TestStep{
Config: testAccInstanceGroupManager_autoHealingPolicies(template, target, igm, hck),
Check: resource.ComposeTestCheckFunc(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to use ComputeTestCheckFunc if you only have one check

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

},
})

if len(manager.AutoHealingPolicies) != 1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered moving these in a TestCheckFunc?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Contributor

@catsby catsby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good 👍

Aside, I'd like to know what you think about adding a "Beta Features" header on the Google Provider documentation page that briefly details that some resources offer beta fields, they are considered ready for use, etc. etc. just so users know what "beta" means to the GCP provider.

If that sounds good, then I'd also request any beta field in a resource documentation then link to this definition.

This is not a blocker though, just something I'd like to hear your thoughts on doing.

@catsby
Copy link
Contributor

catsby commented Aug 2, 2017

Also, I apologize for being a blocker/bottleneck here. Thanks for the patience and transparency with letting me preview/vet how Google plans to introduce beta features 😄

@rileykarson
Copy link
Collaborator Author

@catsby: That sounds good, I'll submit the header and update this page to link to it as a followup PR.

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccInstanceGroupManager -timeout 120m
=== RUN   TestAccInstanceGroupManager_importBasic
--- PASS: TestAccInstanceGroupManager_importBasic (295.09s)
=== RUN   TestAccInstanceGroupManager_importUpdate
--- PASS: TestAccInstanceGroupManager_importUpdate (90.95s)
=== RUN   TestAccInstanceGroupManager_basic
--- PASS: TestAccInstanceGroupManager_basic (253.15s)
=== RUN   TestAccInstanceGroupManager_targetSizeZero
--- PASS: TestAccInstanceGroupManager_targetSizeZero (44.80s)
=== RUN   TestAccInstanceGroupManager_update
--- PASS: TestAccInstanceGroupManager_update (156.22s)
=== RUN   TestAccInstanceGroupManager_updateLifecycle
--- PASS: TestAccInstanceGroupManager_updateLifecycle (104.03s)
=== RUN   TestAccInstanceGroupManager_updateStrategy
--- PASS: TestAccInstanceGroupManager_updateStrategy (210.90s)
=== RUN   TestAccInstanceGroupManager_separateRegions
--- PASS: TestAccInstanceGroupManager_separateRegions (213.87s)
=== RUN   TestAccInstanceGroupManager_autoHealingPolicies
--- PASS: TestAccInstanceGroupManager_autoHealingPolicies (91.30s)
=== RUN   TestAccInstanceGroupManager_selfLinkStability
--- PASS: TestAccInstanceGroupManager_selfLinkStability (123.06s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	1583.506s

@rileykarson rileykarson merged commit 3877b34 into hashicorp:master Aug 2, 2017
z1nkum pushed a commit to z1nkum/terraform-provider-google that referenced this pull request Aug 15, 2017
…hashicorp#249)

* Add support for auto_healing_policies to google_compute_instance_group_manager.

* Add a test for self link stability when a v1 resource uses a versioned resource.

* Add a comment about what the stable self link test does.

* make fmt

* Fixed formatting on new tests.

* Address review comments.

* Fix make vet
negz pushed a commit to negz/terraform-provider-google that referenced this pull request Oct 17, 2017
…hashicorp#249)

* Add support for auto_healing_policies to google_compute_instance_group_manager.

* Add a test for self link stability when a v1 resource uses a versioned resource.

* Add a comment about what the stable self link test does.

* make fmt

* Fixed formatting on new tests.

* Address review comments.

* Fix make vet
chrisst pushed a commit to chrisst/terraform-provider-google that referenced this pull request Nov 9, 2018
…hashicorp#249)

* Add support for auto_healing_policies to google_compute_instance_group_manager.

* Add a test for self link stability when a v1 resource uses a versioned resource.

* Add a comment about what the stable self link test does.

* make fmt

* Fixed formatting on new tests.

* Address review comments.

* Fix make vet
luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019
@ghost
Copy link

ghost commented Mar 31, 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 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

provider/google: Support for managed instance group autohealing
4 participants