Add resource_compute_region_autoscaler #544
Conversation
| return resourceComputeRegionAutoscalerRead(d, meta) | ||
| } | ||
|
|
||
| func flattenRegionAutoscalingPolicy(policy *compute.AutoscalingPolicy) []map[string]interface{} { |
There was a problem hiding this comment.
I'm not sure this needs to be redefined for the regional autoscaler since they both use compute.Autoscaler... anyway. I didn't touch this function actually, just renamed it.
There was a problem hiding this comment.
You can delete this method and reuse the one in the non-regional version
There was a problem hiding this comment.
Thanks Scott for your contribution!
I added comments about reusing code for the regional and zonal autoscaler. I also added comments about problems I noticed that also affect the zonal autoscaler. By doing the refactoring suggested it should fix both at the same time but if that is not the case for some, do you mind also fixing it for the zonal autoscaler too? Thank you.
| return resourceComputeRegionAutoscalerRead(d, meta) | ||
| } | ||
|
|
||
| func flattenRegionAutoscalingPolicy(policy *compute.AutoscalingPolicy) []map[string]interface{} { |
There was a problem hiding this comment.
You can delete this method and reuse the one in the non-regional version
| ForceNew: true, | ||
| }, | ||
|
|
||
| "autoscaling_policy": &schema.Schema{ |
There was a problem hiding this comment.
You can extract this &schema.Schema definition and reuse it for both autoscaler
| } | ||
| } | ||
|
|
||
| func buildRegionAutoscaler(d *schema.ResourceData) (*compute.Autoscaler, error) { |
There was a problem hiding this comment.
You can remove this method and use the buildAutoscaler. In fact, I think you are not even using it and already using the buildAutoscaler.
| d.Set("self_link", scaler.SelfLink) | ||
| d.Set("name", scaler.Name) | ||
| d.Set("target", scaler.Target) | ||
| d.Set("region", regionUrl[len(regionUrl)-1]) |
There was a problem hiding this comment.
You can use the method GetResourceNameFromSelfLink
|
|
||
| "autoscaling_policy": &schema.Schema{ | ||
| Type: schema.TypeList, | ||
| Optional: true, |
There was a problem hiding this comment.
Add MaxItems: 1 since we can only specify at most on autoscaling_policy. It is also not Optional since you must have at least one autoscaling_policy. Please add Required: true
|
|
||
| aspCount := d.Get("autoscaling_policy.#").(int) | ||
| if aspCount != 1 { | ||
| return nil, fmt.Errorf("The autoscaler must have exactly one autoscaling_policy, found %d.", aspCount) |
There was a problem hiding this comment.
You can remove this line once this requirement is enforced by the schema like suggested above. The advantage of enforcing this in the schema is that it fails at terraform plan instead of terraform apply.
| @@ -0,0 +1,262 @@ | |||
| package google | |||
There was a problem hiding this comment.
Can you also add import acceptance tests?
By convention, we put them in a separate file. See `import_compute_autoscaler_test.go"
- Enforce single autoscaling policy requirement at terraform plan time
|
Thanks for the review Vincent! I fixed up the zonal autoscaler on your recommendation and got rid of the duplicate regional functions and both are much more readable now. I also added the import acceptance tests for the regional autoscaler and ran all the acceptance tests again. This was a pleasure to work on, very easy to test and work with! Acceptance Tests➜ google git:(region-autoscaler) ✗ TF_ACC=1 go test -v -run TestAccComputeAutoscaler
=== RUN TestAccComputeAutoscaler_importBasic
--- PASS: TestAccComputeAutoscaler_importBasic (69.92s)
=== RUN TestAccComputeAutoscaler_basic
--- PASS: TestAccComputeAutoscaler_basic (68.78s)
=== RUN TestAccComputeAutoscaler_update
--- PASS: TestAccComputeAutoscaler_update (81.43s)
PASS
ok github.com/terraform-providers/terraform-provider-google/google 220.148s➜ google git:(region-autoscaler) ✗ TF_ACC=1 go test -v -run TestAccComputeRegionAutoscaler
=== RUN TestAccComputeRegionAutoscaler_importBasic
--- PASS: TestAccComputeRegionAutoscaler_importBasic (70.69s)
=== RUN TestAccComputeRegionAutoscaler_basic
--- PASS: TestAccComputeRegionAutoscaler_basic (70.94s)
=== RUN TestAccComputeRegionAutoscaler_update
--- PASS: TestAccComputeRegionAutoscaler_update (83.86s)
PASS
ok github.com/terraform-providers/terraform-provider-google/google 225.508s |
|
Awesome work! Thanks for adding support for this new resource. |
* Add resource_compute_region_autoscaler * Add import acceptance tests, reuse zonal autoscaler code * Enforce single autoscaling policy requirement at terraform plan time
* Add resource_compute_region_autoscaler * Add import acceptance tests, reuse zonal autoscaler code * Enforce single autoscaling policy requirement at terraform plan time
Signed-off-by: Modular Magician <magic-modules@google.com>
|
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 👉 hashibot-feedback@hashicorp.com. Thanks! |
ghost
locked and limited conversation to collaborators
Why
Attempts to fix #535.
It seemed easy enough based on @rosbo 's comment and I think this works but please let me know if I've missed anything! I tried to copy the regular resource_compute_autoscaler as much as possible and luckily they both use the same underlying type.
Tests
I ran acceptance tests pointing at one of my google projects and they all passed.
➜ google git:(region-autoscaler) ✗ TF_ACC=1 go test -v -run TestAccComputeRegionAutoscaler === RUN TestAccComputeRegionAutoscaler_basic --- PASS: TestAccComputeRegionAutoscaler_basic (70.90s) === RUN TestAccComputeRegionAutoscaler_update --- PASS: TestAccComputeRegionAutoscaler_update (83.31s) PASS ok github.com/terraform-providers/terraform-provider-google/google 154.249s