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

fix for dynamodb autoscaling policy import #8397

Merged

Conversation

jdecarli
Copy link
Contributor

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

Fixes #8306

Changes proposed in this pull request:

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

@nywilken
Copy link
Contributor

@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
Copy link
Contributor Author

Thanks for your quick response!

Sure @nywilken I'll work on that

@ghost 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
@jdecarli
Copy link
Contributor Author

jdecarli commented Apr 23, 2019

@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
Copy link
Contributor Author

jdecarli commented May 7, 2019

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

Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

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 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
@nywilken nywilken self-assigned this May 21, 2019
@jdecarli
Copy link
Contributor Author

@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?

func validateAppautoscalingPolicyImportInput(idParts []string) (string, string, string, string, error) {
var serviceNamespace, resourceId, scalableDimension, policyName string

if len(idParts) < 4 || len(idParts) > 6 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

nywilken commented Jun 7, 2019

@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
Copy link
Contributor Author

jdecarli commented Jun 7, 2019

@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
Copy link
Contributor Author

jdecarli commented Jul 9, 2019

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?

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

func TestValidateAppautoscalingPolicyImportInput_positiveCases(t *testing.T) {
Copy link
Contributor

@nywilken nywilken Jul 9, 2019

Choose a reason for hiding this comment

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

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, "/"))
		}
	}
}

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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 Show resolved Hide resolved
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

We should also rebase on to the latest master.

@jdecarli jdecarli requested a review from a team July 18, 2019 19:44
@nywilken nywilken requested a review from bflad July 18, 2019 19:44
@nywilken
Copy link
Contributor

@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.

Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

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

@bflad bflad added the bug Addresses a defect in current functionality. label Jul 18, 2019
@bflad bflad added this to the v2.20.0 milestone Jul 18, 2019
@nywilken nywilken merged commit 00ac3f3 into hashicorp:master Jul 18, 2019
nywilken added a commit that referenced this pull request Jul 18, 2019
@nywilken
Copy link
Contributor

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
Copy link

ghost commented Nov 2, 2019

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 Nov 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can the Regex for importing aws_appautoscaling_policy be relaxed to allow / symbols.
5 participants