fix for dynamodb autoscaling policy import #8397

jdecarli · 2019-04-21T06:11:03Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Changes proposed in this pull request:

Adds a switch to handle DynamoDB, ECS and RDS application autoscaling policy resource ids when import happens

…ds working

nywilken · 2019-04-21T14:41:30Z

@jdecarli thanks for contributing a fix for issue #8306. Quickly looking at the PR, the change looks reasonable but I’ve not officially verified or tested.

With that said, the issue states a failure that does not seem to be represented in the tests. Would you please add the accompanying tests to showcase that the following issue has been fixed.

Importing aws_appautoscaling_policy fails for DynamoDB policy

See our contributing guide for help with writing tests for the provider.

Cheers!

jdecarli · 2019-04-21T15:03:21Z

Thanks for your quick response!

Sure @nywilken I'll work on that

…Input

jdecarli · 2019-04-23T02:24:51Z

@nywilken I did some refactor and added a unit test specifically for the fix.

Acceptance tests:

make testacc TEST=./aws/ TESTARGS='-run=TestValidateAppautoscalingPolicyImportInput'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestValidateAppautoscalingPolicyImportInput -timeout 120m
=== RUN   TestValidateAppautoscalingPolicyImportInput
--- PASS: TestValidateAppautoscalingPolicyImportInput (0.00s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       0.137s

Please, let me know if anything else is required.

Thanks!

jdecarli · 2019-05-07T21:53:01Z

Hi @nywilken does this pr requires anything else in order for merge? I can add any missing piece

nywilken

Hi @jdecarli apologies for the long delay on feedback. The team and I have been focused on TF 0.12 related items the past few weeks. Thank you for sticking with us and for your work on this issue.

I've had a chance to pull down your changes and test. Are you able to run the acceptance tests?

In running the acceptance tests make testacc TESTARGS="-run=TestAccAWSAppautoScalingPolicy_dynamoDb" I get an index out of bounds error on line #441

I've left some suggested changes, and a couple of questions for you. Please let me know if you have any questions or need help implementing the suggested changes.

aws/resource_aws_appautoscaling_policy.go

aws/resource_aws_appautoscaling_policy_test.go

jdecarli · 2019-05-24T20:49:30Z

@nywilken I think I've addressed all suggested changes.

Changes:

serviceNamespace is taken from idParts[0]
TestAccAWSAppautoScalingPolicy_basic unit test moved to line 14
Added negative unit test
Refactored to have all validations inside validateAppautoscalingPolicyImportInput
Fixed TestAccAWSAppautoScalingPolicy_dynamoDb integration test

Please let me know if there's anything else needed?

zarnovican · 2019-05-30T12:38:48Z

aws/resource_aws_appautoscaling_policy.go

+func validateAppautoscalingPolicyImportInput(idParts []string) (string, string, string, string, error) {
+	var serviceNamespace, resourceId, scalableDimension, policyName string
+
+	if len(idParts) < 4 || len(idParts) > 6 {


just checked the doc (aws application-autoscaling describe-scaling-policies help).

resourceId may contain from 0 to 3 "/" characters

scalableDimension seems to be without "/"

policyName can also contain variable number of "/": 0-3

It would be almost better to

choose different field delimiter

or, import by serviceNamespace/policyName (if it is unique in namespace)

or, import by ARN

Hi @zarnovican

Regarding your suggestions...

Choose different field delimiter

If you check the Terraform import documentation you will see the format Terraform chose for the input in all 3 cases (ecs, rds and dynamodb). Acording to that format, I also made 2 unit tests in TestValidateAppautoscalingPolicyImportInput_positiveCases and TestValidateAppautoscalingPolicyImportInput_negativeCases.

I might be wrong (I'm really new to golang), but as far as I can see the only possible delimiter is / (this is the input coming from the import command) and the problem is, as you mention, that it varies depending on the serviceNamespace. This is basically the root of the issue this pr tries to resolve.

Another option I found is to leave the code as it is, truncating the last item in the slice idParts which is the tableName (which also happens to be redundant, because its coming as the third element in the slice). But this will also force us to append this tableName element to the policy, and this will look pretty ugly in the code with something like this:

// truncating last redundant element if idParts[0] == "dynamodb" { idParts = idParts[:len(idParts) -1] } serviceNamespace = idParts[0] resourceId = strings.Join(idParts[1:len(idParts)-2], "/") scalableDimension = idParts[len(idParts)-2] policyName = idParts[len(idParts)-1] // appending tableName to the policy if idParts[0] == "dynamodb" { policyName = policyName + "/" + idParts[2] }

It removes the need for the switch, but adds the truncating and appending.

Import by other fields (serviceNamespace/policyName or ARN)

I think this involves design changes that are out of scope for this issue. Seems like a nice suggestion, but I feel is up to the Terraform team to do some analysis on this. Maybe @nywilken can shed some light on this.

nywilken · 2019-06-07T02:01:44Z

@jdecarli I’m catching up with pending reviews - thanks for continuing to push this forward and for sticking with me. Give me a day to review and think about the latest comments.

jdecarli · 2019-06-07T02:06:56Z

@jdecarli I’m catching up with pending reviews - thanks for continuing to push this forward and for sticking with me. Give me a day to review and think about the latest comments.

Sure, no worries @nywilken . Take your time, and let me know if you want me to make any other change.

jdecarli · 2019-07-09T17:24:42Z

Hi @nywilken

Sorry for pinging again, I just don't want this pr to drift away too much from the codebase

Do you think we need to add anything else?

nywilken · 2019-07-09T21:07:37Z

aws/resource_aws_appautoscaling_policy_test.go

@@ -11,6 +12,85 @@ import (
 	"github.com/hashicorp/terraform/terraform"
 )

+func TestValidateAppautoscalingPolicyImportInput_positiveCases(t *testing.T) {


Here's a slightly different version of these tests. This refactor combines the test cases into one function and relies on an expectedError field to assert that the input validation should fail when expected.

func TestValidateAppautoscalingPolicyImportInput(t *testing.T) { testCases := []struct { input string errorExpected bool expected []string }{ { input: "rds/cluster:id/rds:cluster:ReadReplicaCount/cpu-auto-scaling", expected: []string{"rds", "cluster:id", "rds:cluster:ReadReplicaCount", "cpu-auto-scaling"}, errorExpected: false, }, { input: "ecs/service/clusterName/serviceName/ecs:service:DesiredCount/scale-down", expected: []string{"ecs", "service/clusterName/serviceName", "ecs:service:DesiredCount", "scale-down"}, errorExpected: false, }, { input: "dynamodb/table/tableName/dynamodb:table:ReadCapacityUnits/DynamoDBReadCapacityUtilization:table/tableName", expected: []string{"dynamodb", "table/tableName", "dynamodb:table:ReadCapacityUnits", "DynamoDBReadCapacityUtilization:table/tableName"}, errorExpected: false, }, { input: "dynamodb/missing/parts", errorExpected: true, }, } for _, tc := range testCases { idParts, err := validateAppautoscalingPolicyImportInput(tc.input) if tc.errorExpected == false && err != nil { t.Errorf("validateAppautoscalingPolicyImportInput(%q): resulted in an unexpected error: %s", tc.input, err) } if tc.errorExpected == true && err == nil { t.Errorf("validateAppautoscalingPolicyImportInput(%q): expected an error, but returned successfully", tc.input) } if !reflect.DeepEqual(tc.expected, idParts) { t.Errorf("validateAppautoscalingPolicyImportInput(%q): expected %q, but got %q", tc.input, strings.Join(tc.expected, "/"), strings.Join(idParts, "/")) } } }

nywilken · 2019-07-09T21:34:59Z

aws/resource_aws_appautoscaling_policy.go

@@ -407,18 +407,13 @@ func resourceAwsAppautoscalingPolicyDelete(d *schema.ResourceData, meta interfac

 func resourceAwsAppautoscalingPolicyImport(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
 	idParts := strings.Split(d.Id(), "/")
+	var serviceNamespace, resourceId, scalableDimension, policyName string


What about moving all this string splitting logic into validateAppautoscalingPolicyImportInput to help simplify the Import function?

func resourceAwsAppautoscalingPolicyImport(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { idParts, err := validateAppautoscalingPolicyImportInput(d.Id()) if err != nil { return nil, fmt.Errorf("unexpected format (%q), expected <service-namespace>/<resource-id>/<scalable-dimension>/<policy-name>", d.Id()) } serviceNamespace := idParts[0] resourceId := idParts[1] scalableDimension := idParts[2] policyName := idParts[3] d.Set("service_namespace", serviceNamespace) d.Set("resource_id", resourceId) d.Set("scalable_dimension", scalableDimension) d.Set("name", policyName) d.SetId(policyName) return []*schema.ResourceData{d}, nil }

aws/resource_aws_appautoscaling_policy.go

nywilken

Hi @jdecarli apologies for the long review cycle. I've been away for a couple of weeks. The changes work as expected, thanks for the update. I made a few suggestions around the code itself in an effort simplify things a bit and follow some idiomatic Go constructs for testings. Take a look and let me know if you have any questions.

nywilken · 2019-07-10T00:16:24Z

We should also rebase on to the latest master.

nywilken · 2019-07-18T19:49:50Z

@jdecarli thanks for pushing this change forward. I went ahead and made the last suggested changes and rebased onto master to get this reviewed and merged. @bflad once the latest changes get pushed and it goes green would you mind take a second look.

bflad

LGTM! Those IDs are wild, but nice work on the handling and testing!

nywilken · 2019-07-19T16:25:00Z

The fix to the aws_appautoscaling_policy resource, has been released in version 2.20.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this resource, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2019-11-02T15:19:11Z

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!

fix to fix dynamodb autoscaling policy import and also keep ecs and r…

6f4e59b

…ds working

ghost added size/XS Managed by automation to categorize the size of a PR. service/applicationautoscaling labels Apr 21, 2019

jdecarli mentioned this pull request Apr 21, 2019

Can the Regex for importing aws_appautoscaling_policy be relaxed to allow / symbols. #8306

Closed

refactor/cleanup and unit test for validateAppautoscalingPolicyImport…

8e280ed

…Input

ghost added size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/XS Managed by automation to categorize the size of a PR. labels Apr 23, 2019

updating to use the same table names as the documentation

ca98ae3

nywilken suggested changes May 13, 2019

View reviewed changes

aws/resource_aws_appautoscaling_policy.go Outdated Show resolved Hide resolved

aws/resource_aws_appautoscaling_policy_test.go Outdated Show resolved Hide resolved

aws/resource_aws_appautoscaling_policy_test.go Outdated Show resolved Hide resolved

scampuza approved these changes May 20, 2019

View reviewed changes

refactor and cleanup resourceAwsAppautoscalingPolicyImport

c4f2e1e

nywilken self-assigned this May 21, 2019

jdecarli added 2 commits May 22, 2019 00:09

fixing typo/error in test logic

b0e095c

fixing integration test (TestAccAWSAppautoScalingPolicy_dynamoDb)

7d86d43

zarnovican reviewed May 30, 2019

View reviewed changes

nywilken reviewed Jul 10, 2019

View reviewed changes

resource/aws_appautoscaling_policy: Update import validation support

7ecc6c1

jdecarli requested a review from a team July 18, 2019 19:44

nywilken requested a review from bflad July 18, 2019 19:44

Merge branch 'master' into fix-for-dynamodb-policy-import-issue-8306

a04400e

bflad approved these changes Jul 18, 2019

View reviewed changes

bflad added the bug Addresses a defect in current functionality. label Jul 18, 2019

bflad added this to the v2.20.0 milestone Jul 18, 2019

nywilken merged commit 00ac3f3 into hashicorp:master Jul 18, 2019

nywilken added a commit that referenced this pull request Jul 18, 2019

update CHANGELOG for #8397

afe5288

ghost locked and limited conversation to collaborators Nov 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix for dynamodb autoscaling policy import #8397

fix for dynamodb autoscaling policy import #8397

jdecarli commented Apr 21, 2019

nywilken commented Apr 21, 2019

jdecarli commented Apr 21, 2019

jdecarli commented Apr 23, 2019 •

edited

Loading

jdecarli commented May 7, 2019

nywilken left a comment

jdecarli commented May 24, 2019

zarnovican May 30, 2019

jdecarli Jun 1, 2019

nywilken commented Jun 7, 2019

jdecarli commented Jun 7, 2019

jdecarli commented Jul 9, 2019

nywilken Jul 9, 2019 •

edited

Loading

nywilken Jul 9, 2019

nywilken left a comment

nywilken commented Jul 10, 2019

nywilken commented Jul 18, 2019

bflad left a comment

nywilken commented Jul 19, 2019

ghost commented Nov 2, 2019

fix for dynamodb autoscaling policy import #8397

fix for dynamodb autoscaling policy import #8397

Conversation

jdecarli commented Apr 21, 2019

Community Note

nywilken commented Apr 21, 2019

jdecarli commented Apr 21, 2019

jdecarli commented Apr 23, 2019 • edited Loading

jdecarli commented May 7, 2019

nywilken left a comment

Choose a reason for hiding this comment

jdecarli commented May 24, 2019

zarnovican May 30, 2019

Choose a reason for hiding this comment

jdecarli Jun 1, 2019

Choose a reason for hiding this comment

Choose different field delimiter

Import by other fields (serviceNamespace/policyName or ARN)

nywilken commented Jun 7, 2019

jdecarli commented Jun 7, 2019

jdecarli commented Jul 9, 2019

nywilken Jul 9, 2019 • edited Loading

Choose a reason for hiding this comment

nywilken Jul 9, 2019

Choose a reason for hiding this comment

nywilken left a comment

Choose a reason for hiding this comment

nywilken commented Jul 10, 2019

nywilken commented Jul 18, 2019

bflad left a comment

Choose a reason for hiding this comment

nywilken commented Jul 19, 2019

ghost commented Nov 2, 2019

jdecarli commented Apr 23, 2019 •

edited

Loading

nywilken Jul 9, 2019 •

edited

Loading