add new compute_instance_from_template resource #1652
Conversation
|
I'm reviewing the code right now, but in case I get distracted while doing that, just wanted to jump in here and say I agree with making this a separate resource. |
There was a problem hiding this comment.
Some nitpicks, but I think the majority of the code is OK. I think the only thing I'd block on is dropping create_timeout, if there's a better way to do ForceSendFields you can think of (if not, I'll get over it), and renaming the computeInstance function.
Overall, I'm not super comfortable with this PR, just because of the number of implicit dependencies it has on compute_instance and on the way we name things, but on the other hand, I can't really think of a better alternative, so I won't block on the fact that I'm uncomfortable with the implicit dependencies.
google/resource_compute_instance.go
Outdated
| "Error loading zone '%s': %s", z, err) | ||
| } | ||
|
|
||
| func computeInstance(project string, zone *compute.Zone, d *schema.ResourceData, config *Config) (*computeBeta.Instance, error) { |
There was a problem hiding this comment.
Don't we usually call these something like expandComputeInstance? computeInstance alone has no verb, which makes it hard to tell what the function does, IMO. buildComputeInstance, expandComputeInstance, etc. may be a clearer choice here.
google/resource_compute_instance.go
Outdated
| if len(instance.Labels) > 0 { | ||
| d.Set("labels", instance.Labels) | ||
| } | ||
| d.Set("labels", instance.Labels) |
There was a problem hiding this comment.
If we're changing this anyways, do you mind just checking the error here?
| } | ||
| } | ||
|
|
||
| func computeInstanceFromTemplateSchema() map[string]*schema.Schema { |
There was a problem hiding this comment.
This makes me really nervous, just because I worry about changes to compute_instance's schema that don't keep in mind that this happens, or changes to e.g. resourceComputeInstanceUpdate that rely on the schema for compute_instance having some non-Computed value.
But as long as we have good tests, I guess I can't think of a good reason not to go ahead with this, especially since I don't have a better idea.
There was a problem hiding this comment.
Yeah, it was either this or copy/paste and then have to keep them both up-to-date manually, and this seemed to me like the better option.
There was a problem hiding this comment.
Agreed. The nice thing about this is we know we want those fields, so if we decide in the future to go a different direction with this, we can do it without a breaking change. But yeah, this is the best way I can think of to do it right now.
| createTimeout := int(d.Timeout(schema.TimeoutCreate).Minutes()) | ||
| if v, ok := d.GetOk("create_timeout"); ok && v != 4 { | ||
| createTimeout = v.(int) | ||
| } |
There was a problem hiding this comment.
Is there a reason to port over this behavior we want to get rid of to a new resource? I think we could probably just not use create_timeout here.
| } | ||
|
|
||
| if _, exists := d.GetOkExists(f); exists { | ||
| instance.ForceSendFields = append(instance.ForceSendFields, strcase.UpperCamelCase(f)) |
There was a problem hiding this comment.
:/ I can see this breaking when someone adds a field to compute_instance that isn't a direct 1:1 snake_case version of the API field.
There was a problem hiding this comment.
Fair. I took a look at our current fields, and as far as I can tell, things that would potentially be overridden to their zero value are indeed snake_case versions of the API fields. Given that we're generally trying to do that sort of thing anyway for new fields, I'm not too concerned, but I added a comment.
There was a problem hiding this comment.
I assume the API returns a good error if a field that doesn't match a field is set in ForceSendFields? I'm gonna test that real quick. As long as a field that doesn't match up isn't silently ignored, I think this is a manageable solution. Nested values scare me a little, and I think over time we may want to add a test that, e.g., checks all the properties work out to the same strings as ForceSendFields, but that's probably not a project for today.
There was a problem hiding this comment.
Nope, it's silently ignored. I could instead directly set the fields that we expect to be able to be overridden to zero values, but I honestly think that's just about as error prone as this is.
There was a problem hiding this comment.
The API does, actually, just silently ignore invalid fields. :/ I've opened googleapis/google-api-go-client#280 to see if I can't get the client to throw an error on invalid fields. Otherwise, this may be something worth bringing in internally, but I suppose I'm happy to punt on that, as we have test coverage, and don't have perfect ForceSendFields support elsewhere, anyways, so the class of bugs this would enable is a class of bugs we already accept.
Sorry, this is basically a long comment explaining for future-Paddy/future maintainers why this was an acceptable risk, in case someone's doing archaeology for a bug. :)
This was done as its own resource as suggested in slack, since we don't have the option of making all fields Computed in google_compute_instance. There's precedent in the aws provider for this sort of thing (see ami_copy, ami_from_instance). When I started working on this I assumed I could do it in the compute_instance resource and so I went ahead and reordered the schema to make it easier to work with in the future. Now it's not quite relevant, but I left it in as its own commit that can be looked at separately from the other changes. Fixes hashicorp#1582.
|
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
locked and limited conversation to collaborators
This was done as its own resource as suggested in slack, since we don't have the option of making all fields Computed in google_compute_instance. There's precedent in the aws provider for this sort of thing (see ami_copy, ami_from_instance).
When I started working on this I assumed I could do it in the compute_instance resource and so I went ahead and reordered the schema to make it easier to work with in the future. Now it's not quite relevant, but I left it in as its own commit that can be looked at separately from the other changes.
Fixes #1582.