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.

Sign up for GitHub

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

Jump to bottom

resource/aws_autoscaling_policy: Add support for target_tracking_configuration #2611

Merged
merged 3 commits into from
Feb 12, 2018
Merged

resource/aws_autoscaling_policy: Add support for target_tracking_configuration #2611

merged 3 commits into from
Feb 12, 2018

Conversation

atsushi-ishibashi
Copy link
Contributor

Support: #1156

TF_ACC=1 go test ./aws -v -run=TestAccAWSAutoscalingPolicy_TargetTrack -timeout 120m
=== RUN   TestAccAWSAutoscalingPolicy_TargetTrack_Predefined
--- PASS: TestAccAWSAutoscalingPolicy_TargetTrack_Predefined (87.74s)
=== RUN   TestAccAWSAutoscalingPolicy_TargetTrack_Custom
--- PASS: TestAccAWSAutoscalingPolicy_TargetTrack_Custom (82.00s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	169.792s

adjustment_type isn't supported if the policy type is TargetTrackingScaling so I changed Optional

@Ninir Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Dec 9, 2017
@atsushi-ishibashi
Copy link
Contributor Author

What is the status?

@bflad bflad added the service/autoscaling Issues and PRs that pertain to the autoscaling service. label Jan 17, 2018
@krrrr38 krrrr38 mentioned this pull request Jan 28, 2018
Closed
4 tasks
radeksimko
radeksimko suggested changes Feb 8, 2018
View reviewed changes
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Hi @atsushi-ishibashi
thanks for the PR and sorry for the delay on our side in processing it.

I left you some questions/comments to address, the important ones around schema mainly.

aws/resource_aws_autoscaling_policy.go Outdated
@@ -93,6 +93,81 @@ func resourceAwsAutoscalingPolicy() *schema.Resource {
},
Set: resourceAwsAutoscalingScalingAdjustmentHash,
},
"target_tracking_configuration": &schema.Schema{
Type: schema.TypeSet,
Copy link
Member

Choose a reason for hiding this comment

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

It looks like target_tracking_configuration is a singleton - i.e. we'll only ever have a single instance of it since it maps to a struct, not a map nor a slice. Do you mind changing it to schema.TypeList and attaching MaxItems: 1? We generally prefer TypeList for singletons because it's much easier to work with the field without the hash and because there's really no ordering problem as there's nothing to order, so no reason for set.

aws/resource_aws_autoscaling_policy.go Outdated
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"predefined_metric_specification": &schema.Schema{
Type: schema.TypeSet,
Copy link
Member

Choose a reason for hiding this comment

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

Likewise - as above ^

aws/resource_aws_autoscaling_policy.go Outdated
"predefined_metric_specification": &schema.Schema{
Type: schema.TypeSet,
Optional: true,
ConflictsWith: []string{"target_tracking_configuration.customized_metric_specification"},
Copy link
Member

Choose a reason for hiding this comment

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

I think this won't actually work, because the address isn't correct. The real address would be target_tracking_configuration.0.customized_metric_specification (if we changed target_tracking_configuration to TypeList).

aws/resource_aws_autoscaling_policy.go Outdated
},
},
"customized_metric_specification": &schema.Schema{
Type: schema.TypeSet,
Copy link
Member

Choose a reason for hiding this comment

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

Likewise - as above ^

aws/resource_aws_autoscaling_policy.go Outdated
"customized_metric_specification": &schema.Schema{
Type: schema.TypeSet,
Optional: true,
ConflictsWith: []string{"target_tracking_configuration.predefined_metric_specification"},
Copy link
Member

Choose a reason for hiding this comment

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

Likewise - as above ^

aws/resource_aws_autoscaling_policy_test.go Outdated
return fmt.Sprintf(`
resource "aws_launch_configuration" "test" {
name = "tf-test-%s"
image_id = "ami-21f78e11"
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Do you mind using the AMI data source here instead, so we don't need to keep updating the ID over time as often?

aws/resource_aws_autoscaling_policy_test.go Outdated
}

resource "aws_autoscaling_group" "test" {
availability_zones = ["us-west-2a"]
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind using the AZ data source here to make the test region-agnostic?
https://www.terraform.io/docs/providers/aws/d/availability_zones.html

aws/resource_aws_autoscaling_policy_test.go Outdated
resource "aws_launch_configuration" "test" {
name = "tf-test-%s"
image_id = "ami-21f78e11"
instance_type = "t1.micro"
Copy link
Member

Choose a reason for hiding this comment

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

AFAIK t1 family of instances has been deprecated - do you mind changing this to any low-price size of current family? e.g. t2.micro? https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/concepts_micro_instances.html

aws/resource_aws_autoscaling_policy_test.go Outdated
resource "aws_launch_configuration" "test" {
name = "tf-test-%s"
image_id = "ami-21f78e11"
instance_type = "t1.micro"
Copy link
Member

Choose a reason for hiding this comment

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

Likewise - as above ^

aws/resource_aws_autoscaling_policy_test.go Outdated
}

resource "aws_autoscaling_group" "test" {
availability_zones = ["us-west-2a"]
Copy link
Member

Choose a reason for hiding this comment

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

Likewise - as above ^

@radeksimko radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 8, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 10, 2018
@atsushi-ishibashi
Copy link
Contributor Author

@radeksimko Ok👍

TF_ACC=1 go test ./aws -v -run=TestAccAWSAutoscalingPolicy_TargetTrack* -timeout 120m
=== RUN   TestAccAWSAutoscalingPolicy_TargetTrack_Predefined
--- PASS: TestAccAWSAutoscalingPolicy_TargetTrack_Predefined (110.02s)
=== RUN   TestAccAWSAutoscalingPolicy_TargetTrack_Custom
--- PASS: TestAccAWSAutoscalingPolicy_TargetTrack_Custom (105.76s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	215.812s

@radeksimko radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Feb 12, 2018
radeksimko
radeksimko approved these changes Feb 12, 2018
View reviewed changes
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

LGTM, thanks 👍

@radeksimko radeksimko changed the title r/aws_autoscaling_policy: Support TargetTrackingConfiguration r/aws_autoscaling_policy: Add support for target_tracking_configuration Feb 12, 2018
@radeksimko radeksimko changed the title r/aws_autoscaling_policy: Add support for target_tracking_configuration resource/aws_autoscaling_policy: Add support for target_tracking_configuration Feb 12, 2018
@radeksimko radeksimko merged commit 85cee1c into hashicorp:master Feb 12, 2018
@radeksimko radeksimko added this to the v1.10.0 milestone Feb 12, 2018
@radeksimko radeksimko mentioned this pull request Feb 12, 2018
Closed
@vladholubiev vladholubiev mentioned this pull request Feb 25, 2018
Closed
@bflad
Copy link
Contributor

bflad commented Feb 27, 2018

This has been released in version 1.10.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Apr 7, 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. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@radeksimko radeksimko radeksimko approved these changes

Assignees
No one assigned
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/autoscaling Issues and PRs that pertain to the autoscaling service. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
v1.10.0
Development

Successfully merging this pull request may close these issues.
4 participants
@atsushi-ishibashi @bflad @radeksimko @Ninir