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 mode enum and scale down controls for Compute AutoScaler #3693

Merged
merged 4 commits into from
Jun 23, 2020

Conversation

rajatvig
Copy link
Contributor

@rajatvig rajatvig commented Jun 19, 2020

Resolves the following two issues
Fixes: hashicorp/terraform-provider-google#6363
Fixes: hashicorp/terraform-provider-google#6636
Fixes: hashicorp/terraform-provider-google#5521

Release Note Template for Downstream PRs (will be copied)

compute: Added `mode` to `google_compute_autoscaler` `autoscaling_policy`
compute: Added `scale_down_control` for `google_compute_autoscaler` `autoscaling_policy` (beta only)

@modular-magician
Copy link
Collaborator

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@slevenick, please review this PR or find an appropriate assignee.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 32 insertions(+))
Terraform Beta: Diff ( 6 files changed, 260 insertions(+), 6 deletions(-))
Ansible: Diff ( 2 files changed, 19 insertions(+))
TF Conversion: Diff ( 1 file changed, 11 insertions(+))
Inspec: Diff ( 6 files changed, 94 insertions(+), 2 deletions(-))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=121665"

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 2 files changed, 32 insertions(+))
Terraform Beta: Diff ( 4 files changed, 260 insertions(+), 6 deletions(-))
Ansible: Diff ( 2 files changed, 20 insertions(+))
TF Conversion: Diff ( 1 file changed, 11 insertions(+))
Inspec: Diff ( 6 files changed, 103 insertions(+), 2 deletions(-))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=121666"

@slevenick
Copy link
Contributor

Hey @rajatvig thanks for the contribution!

I'm seeing tests that are failing because of this:

TestAccComputeAutoscaler_update: testing.go:674: Step 0 error: After applying this step, the plan was not empty:
DIFF:
UPDATE: google_compute_autoscaler.foobar
autoscaling_policy.#:                              "1" => "1"
...
autoscaling_policy.0.mode:                         "ON" => ""

I believe the mode field will need to be set to either have a default value that matches the default on the API, or marked as default_from_api which marks the field as Computed. A similar thing may be true for the scale down controls

Copy link
Contributor

@slevenick slevenick left a comment

Choose a reason for hiding this comment

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

See above comment

@rajatvig
Copy link
Contributor Author

@slevenick Think I did what you suggested, though not very sure. Is there a way I can run those tests to be sure?

@modular-magician
Copy link
Collaborator

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@danawillow, please review this PR or find an appropriate assignee.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 35 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 5 files changed, 264 insertions(+), 7 deletions(-))
Ansible: Diff ( 2 files changed, 21 insertions(+))
TF Conversion: Diff ( 1 file changed, 11 insertions(+))
Inspec: Diff ( 6 files changed, 103 insertions(+), 2 deletions(-))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=121688"

@danawillow danawillow removed their request for review June 19, 2020 23:55
@slevenick
Copy link
Contributor

@slevenick Think I did what you suggested, though not very sure. Is there a way I can run those tests to be sure?

Yeah, first you will want to generate the downstream provider: https://github.com/GoogleCloudPlatform/magic-modules#generating-downstream-tools

Then you can run the tests from that generated provider: https://github.com/terraform-providers/terraform-provider-google/blob/master/.github/CONTRIBUTING.md#tests

@rajatvig
Copy link
Contributor Author

@slevenick Thanks for the pointers. I ran the particular test locally after generating code for the beta provider and it passed.

@rajatvig rajatvig requested a review from slevenick June 20, 2020 10:10
Copy link
Contributor

@slevenick slevenick left a comment

Choose a reason for hiding this comment

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

Can you also add a test that includes these fields to https://github.com/GoogleCloudPlatform/magic-modules/blob/master/third_party/terraform/tests/resource_compute_autoscaler_test.go.erb

You can either add a new test or add these fields to the existing test. It would be best if at least one of these fields gets updated during the test

and outages due to abrupt scale-in events
properties:
- !ruby/object:Api::Type::NestedObject
name: 'maxScaledDownReplicas'
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 add at_least_one_of: for this?

It helps us prevent issues around empty blocks by requiring at least one field per block is specified.

In this case I believe it will look like:

at_least_one_of:
  - scale_down_control.0.max_scaled_down_replicas.0.fixed
  - scale_down_control.0.max_scaled_down_replicas.0.percent

And then one to scaleDownControl that looks like:

at_least_one_of:
  - scale_down_control.0.max_scaled_down_replicas
  - scale_down_control.0.time_window_sec

For example: https://github.com/GoogleCloudPlatform/magic-modules/blob/master/products/compute/api.yaml#L757

@modular-magician
Copy link
Collaborator

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@slevenick, please review this PR or find an appropriate assignee.

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 35 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 6 files changed, 316 insertions(+), 7 deletions(-))
Ansible: Diff ( 2 files changed, 21 insertions(+))
TF Conversion: Diff ( 1 file changed, 11 insertions(+))
Inspec: Diff ( 6 files changed, 103 insertions(+), 2 deletions(-))

@modular-magician
Copy link
Collaborator

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=122386"

Copy link
Contributor

@slevenick slevenick left a comment

Choose a reason for hiding this comment

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

Thanks for the addition! I modified the notes a bit to fit our changelog format

@slevenick slevenick merged commit 52faa9a into GoogleCloudPlatform:master Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants