Add support for auto_healing_policies to google_compute_instance_group_manager #198

rileykarson · 2017-07-14T20:54:20Z

Add support for auto_healing_policies to google_compute_instance_group_manager; Fixes #48
Add support for a beta GCP feature to a Google Provider resource; part of #93

Terraform manages v0beta and v0alpha Compute resources by using non-v1 APIs to create and manage resources when necessary, automatically using the most restrictive API it can. Users don't need to think about what version they are using, or perform complicated operations to manage their state files.

For reviewing, the best way to view the merge changes is by filtering for only the last two commits in the change view; Github is able to show just those changes and hides the merged commits.

…pport (#129) * Refactor resource_compute_instance_group_manager for multi version support. * Minor changes based on review. * Removed type-specific API version conversion functions.

… a change.

…p_manager.

…sion resources don't see diffs.

Merge master into beta-resources

danawillow · 2017-07-14T21:58:36Z

Assuming this is just merging that branch into master with all the different changes I've reviewed, I'm ok with this. However, since it's a pretty big infrastructure change, it may be worth it to check in slack whether anyone from HashiCorp wants to review it first or whether they're ok with our judgment.

evandbrown · 2017-07-17T17:38:23Z

google/compute_beta_operation.go

+		case ComputeBetaOperationWaitZone:
+			op, err = w.Service.ZoneOperations.Get(
+				w.Project, w.Zone, w.Op.Name).Do()
+		default:


In the default case, should we consider the operation type "unknown" rather than "invalid"? And then update the error message to something like "Unknown wait condition %#v encountered. ComputeBetaOperationWaiter should be updated by maintainers to include this new operation type." or something?

ComputeBetaOperation is a copy of the existing ComputeOperation where we sub compute/v0beta's Operation type for compute/v1's Operation type. These changes would also need to be applied to compute_operation.go as well as compute_beta_operation.go. and I don't think that kind of change makes sense in the context of this PR.

Improving Operations is something that I've been thinking about with #187, which would introduce a standard Operation type for all versioned compute resources (such as storing them all as v1 Operations), and a refactoring of compute_operation.go that was pulled into #191.

evandbrown · 2017-07-17T17:42:00Z

google/compute_beta_operation.go

+func (e ComputeBetaOperationError) Error() string {
+	var buf bytes.Buffer
+
+	for _, err := range e.Errors {


If the ComputeBetaOperationError has multiple errors, and any of the errors have a newline in the message, it will be hard to differentiate them. Maybe take the index of the range and prefix it to the buffer?

See above. If it's something you think would be valuable for compute_operation.go as well, feel free to comment with it on #191.

evandbrown · 2017-07-24T19:21:23Z

google/api_versions.go

@@ -0,0 +1,99 @@
+package google


Need license header

Are license headers something that we include in any of these files? Looking over provider.go, config.go, and a few resources, I can't see one included elsewhere.

Ahh, I guess not. But we should. I'll file an issue.

evandbrown · 2017-07-24T19:23:45Z

google/compute_beta_operation.go

+		return fmt.Errorf("Error waiting for %s: %s", activity, err)
+	}
+
+	op = opRaw.(*computeBeta.Operation)


Worth checking that the type assertion succeeds here? Like op, ok := opRaw.(...)

evandbrown · 2017-07-24T19:24:38Z

google/compute_beta_operation.go

+}
+
+func computeBetaOperationWaitZone(config *Config, op *computeBeta.Operation, project string, zone, activity string) error {
+	return computeBetaOperationWaitZoneTime(config, op, project, zone, 4, activity)


Should we make the time vars more configurable/obvious?

See above. If we want to expose a better interface, that's something we should consider for #191.

evandbrown · 2017-07-24T19:25:14Z

google/compute_beta_operation.go

+	return nil
+}
+
+func computeBetaOperationWaitZone(config *Config, op *computeBeta.Operation, project string, zone, activity string) error {


project, zone, activity string (remove the string after project)

See above/changes made in #191.

evandbrown · 2017-07-24T19:25:47Z

google/compute_beta_operation.go

+		Type:    ComputeBetaOperationWaitZone,
+	}
+	state := w.Conf()
+	state.Delay = 10 * time.Second


See previous about configuring timeouts in one place.

See above. Change made in https://github.com/terraform-providers/terraform-provider-google/pull/191/files#diff-ef92ceb02a5a2c0c742317597b0bc870R146

evandbrown · 2017-07-24T19:34:13Z

google/compute_shared_operation.go

+}
+
+func computeSharedOperationWaitZoneTime(config *Config, op interface{}, project string, zone string, minutes int, activity string) error {
+	if op == nil {


nil check shouldn't be necessary as the type switch's default will catch it.

Added a nil check as a case in #234 instead of having it at the top.

rileykarson · 2017-07-24T20:23:30Z

Closing this PR in favour of splitting it in 2, adding Beta support to google_compute_instance_group_manager in #234 followed by adding auto_healing_policies in a later PR.

ghost · 2018-11-16T13:57:12Z

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!

rileykarson and others added 12 commits July 6, 2017 17:32

Refactor resource_compute_instance_group_manager for multi version su…

b9dcc6c

…pport (#129) * Refactor resource_compute_instance_group_manager for multi version support. * Minor changes based on review. * Removed type-specific API version conversion functions.

Add support for Beta operations.

7461e6e

Add v0beta support to google_compute_instance_group_manager.

3f00096

Renamed Key to Feature, added comments & updated some parameter names.

5048095

Fix code and tests for version finder to match fields that don't have…

355e005

… a change.

Add support for auto_healing_policies to google_compute_instance_grou…

fa6b125

…p_manager.

Move test configuration above helper method.

912bfc7

Store non-v1 resources' self links as v1 so that dependent single-ver…

94f9d35

…sion resources don't see diffs.

Add a test with a single version resource linking to a versioned one.

4155f18

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

d039da9

Merge branch 'master' into beta-resources-merge-master

3083f38

Merge pull request #197 from rileykarson/beta-resources-merge-master

e6df699

Merge master into beta-resources

rileykarson requested a review from danawillow July 14, 2017 20:54

rileykarson mentioned this pull request Jul 24, 2017

Add versioned Beta support to google_compute_instance_group_manager #234

Merged

evandbrown reviewed Jul 24, 2017

View reviewed changes

rileykarson closed this Jul 24, 2017

rileykarson mentioned this pull request Aug 4, 2017

[WIP] Multiversioned Compute Service & Versioned Operations #187

Closed

rileykarson deleted the beta-resources branch October 29, 2018 19:27

ghost locked and limited conversation to collaborators Nov 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for auto_healing_policies to google_compute_instance_group_manager #198

Add support for auto_healing_policies to google_compute_instance_group_manager #198

rileykarson commented Jul 14, 2017

danawillow commented Jul 14, 2017

evandbrown Jul 17, 2017

rileykarson Jul 24, 2017

evandbrown Jul 17, 2017

rileykarson Jul 24, 2017

evandbrown Jul 24, 2017

rileykarson Jul 24, 2017

evandbrown Jul 25, 2017

evandbrown Jul 24, 2017

rileykarson Jul 24, 2017

evandbrown Jul 24, 2017

rileykarson Jul 24, 2017

evandbrown Jul 24, 2017

rileykarson Jul 24, 2017

evandbrown Jul 24, 2017

rileykarson Jul 24, 2017

evandbrown Jul 24, 2017

rileykarson Jul 24, 2017

rileykarson commented Jul 24, 2017

ghost commented Nov 16, 2018

Add support for auto_healing_policies to google_compute_instance_group_manager #198

Add support for auto_healing_policies to google_compute_instance_group_manager #198

Conversation

rileykarson commented Jul 14, 2017

danawillow commented Jul 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rileykarson commented Jul 24, 2017

ghost commented Nov 16, 2018