r/aws_backup_selection: Adding resource to manage selections for AWS Backup plans #7382

slapula · 2019-01-30T05:00:16Z

Changes proposed in this pull request:

Adding resource to manage AWS Backup plans: aws_backup_selection

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAwsBackupSelection_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAwsBackupSelection_ -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsBackupSelection_basic
=== PAUSE TestAccAwsBackupSelection_basic
=== RUN   TestAccAwsBackupSelection_withTags
=== PAUSE TestAccAwsBackupSelection_withTags
=== RUN   TestAccAwsBackupSelection_withResources
=== PAUSE TestAccAwsBackupSelection_withResources
=== CONT  TestAccAwsBackupSelection_basic
=== CONT  TestAccAwsBackupSelection_withResources
=== CONT  TestAccAwsBackupSelection_withTags
--- PASS: TestAccAwsBackupSelection_withTags (14.03s)
--- PASS: TestAccAwsBackupSelection_withResources (14.40s)
--- PASS: TestAccAwsBackupSelection_basic (14.41s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	14.430s

…Backup plans

…ction

bflad

Hi @slapula 👋 Thanks for another good contribution. Initial feedback below. Please reach out with any questions or if you do not have time to implement the items.

aws/resource_aws_backup_selection.go

bflad · 2019-03-29T02:23:28Z

aws/resource_aws_backup_selection.go

+						},
+					},
+				},
+				Set: resourceAwsConditionTagHash,


We should simplify this attribute by removing the custom Set function and allowing Terraform to use its default.

Does it matter if this resource is using these tags as a parameter to make backup plan selections vs. the normal tag/untag operations that Terraform handles? That's what I noticed when I first planned this out but I will admit I didn't try using the default functions first because I thought they wouldn't work in this context.

I must admit I'm not sure what you're referring to? 😅 All Terraform schema definitions are unrelated and the default TypeSet Set function applies to any TypeSet attribute. We happen to use a tags TypeMap for most Terraform AWS Provider resources as the resource tagging attribute, but that is only a convention to make it simpler for operators.

If you would like to clear up any confusion between a tag configuration block here and the typical tags map argument across the provider, you can always switch the naming here to selection_tag or something. It won't have any bearing on the TypeSet Set function either which way though.

Now that I have this working without the custom hash function this all makes sense. I think I misinterpreted your initial comment to imply that I use tagSchema() which doesn't make sense. Oops! Anyway, thanks for the clarification :-)

aws/resource_aws_backup_selection.go

aws/resource_aws_backup_selection_test.go

website/docs/r/backup_selection.html.markdown

asweat-pubfactory · 2019-03-29T15:50:02Z

Just curious is this scheduled for any upcoming releases? We've got the backup_vault, and backup_plan setup, super excited to have backup_selection. 👍

amirgrois · 2019-03-29T21:36:37Z

waiting as well

slapula · 2019-03-29T22:23:04Z

Sit tight guys, I need to make some changes then this will be up for another review. We'll merge this once we get all the kinks worked out.

slapula · 2019-03-30T21:49:32Z

@bflad I believe I've addressed all of your comments:

$ make testacc TESTARGS='-run=TestAccAwsBackupSelection_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAwsBackupSelection_ -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsBackupSelection_basic
=== PAUSE TestAccAwsBackupSelection_basic
=== RUN   TestAccAwsBackupSelection_disappears
=== PAUSE TestAccAwsBackupSelection_disappears
=== RUN   TestAccAwsBackupSelection_withTags
=== PAUSE TestAccAwsBackupSelection_withTags
=== RUN   TestAccAwsBackupSelection_withResources
=== PAUSE TestAccAwsBackupSelection_withResources
=== CONT  TestAccAwsBackupSelection_basic
=== CONT  TestAccAwsBackupSelection_withResources
=== CONT  TestAccAwsBackupSelection_disappears
=== CONT  TestAccAwsBackupSelection_withTags
--- PASS: TestAccAwsBackupSelection_disappears (19.11s)
--- PASS: TestAccAwsBackupSelection_basic (19.12s)
--- PASS: TestAccAwsBackupSelection_withResources (19.84s)
--- PASS: TestAccAwsBackupSelection_withTags (20.62s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	20.634s

bflad · 2019-04-01T15:07:43Z

From quick glance it looks like removing Set function from the TypeSet configuration block might be the only item, although I think renaming the attribute to selection_tag might not be a bad idea either. 👍

slapula · 2019-04-04T02:11:20Z

@bflad I went ahead and changed the attribute to selection_tag. I like that and that makes it unique compared to the other uses of tag in this provider's codebase.

bflad

Thanks, @slapula 🚀 Looks good after the two below minor adjustments.

Output from acceptance testing:

--- PASS: TestAccAwsBackupSelection_withResources (16.55s)
--- PASS: TestAccAwsBackupSelection_basic (17.21s)
--- PASS: TestAccAwsBackupSelection_disappears (17.70s)
--- PASS: TestAccAwsBackupSelection_withTags (17.79s)

bflad · 2019-04-04T11:49:10Z

aws/resource_aws_backup_selection_test.go

+  }
+
+  resources = [
+    "arn:aws:ec2:us-east-1:${data.aws_caller_identity.current.account_id}:volume/"


The acceptance testing runs in us-west-2 by default:

https://github.com/terraform-providers/terraform-provider-aws/blob/26fea575f2100d68307e063a8b1c590d8de622d6/aws/provider_test.go#L181-L185

--- FAIL: TestAccAwsBackupSelection_withResources (13.48s) testing.go:538: Step 0 error: Error applying: 1 error occurred: * aws_backup_selection.test: 1 error occurred: * aws_backup_selection.test: error creating Backup Selection: InvalidParameterValueException: Invalid ARN: arn:aws:elasticfilesystem:us-east-1:--OMITTED--:file-system/. Expected region is us-west-2 status code: 400, request id: 8c64a59c-7b8c-46de-8e7e-6fa1a7879b3a --- FAIL: TestAccAwsBackupSelection_disappears (13.53s) testing.go:538: Step 0 error: Error applying: 1 error occurred: * aws_backup_selection.test: 1 error occurred: * aws_backup_selection.test: error creating Backup Selection: InvalidParameterValueException: Invalid ARN: arn:aws:ec2:us-east-1:--OMITTED--:volume/. Expected region is us-west-2 status code: 400, request id: bb483272-0b15-4978-88ab-0a5cf2400302 --- FAIL: TestAccAwsBackupSelection_basic (13.60s) testing.go:538: Step 0 error: Error applying: 1 error occurred: * aws_backup_selection.test: 1 error occurred: * aws_backup_selection.test: error creating Backup Selection: InvalidParameterValueException: Invalid ARN: arn:aws:ec2:us-east-1:--OMITTED--:volume/. Expected region is us-west-2 status code: 400, request id: 6e4c8dda-6cc8-4d63-be62-b4b156e4c30b --- FAIL: TestAccAwsBackupSelection_withTags (13.66s) testing.go:538: Step 0 error: Error applying: 1 error occurred: * aws_backup_selection.test: 1 error occurred: * aws_backup_selection.test: error creating Backup Selection: InvalidParameterValueException: Invalid ARN: arn:aws:ec2:us-east-1:--OMITTED--:volume/. Expected region is us-west-2 status code: 400, request id: a1229860-b1f7-4462-9b22-68d46cf62e31

We generally prefer using the aws_partition and aws_region data sources in this case to make the testing region and partition agnostic:

data "aws_partition" "current" {} data "aws_region" "current" {} // ... "arn:${data.aws_partition.current.partition}:ec2:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:volume/"

bflad · 2019-04-04T11:51:01Z

aws/resource_aws_backup_selection.go

+				},
+			},
+			"resources": {
+				Type:     schema.TypeList,


The API seems to be unordered for this attribute:

--- FAIL: TestAccAwsBackupSelection_withResources (14.15s) testing.go:538: Step 0 error: After applying this step, the plan was not empty: DIFF: DESTROY/CREATE: aws_backup_selection.test iam_role_arn: "arn:aws:iam::123456789012:role/service-role/AWSBackupDefaultServiceRole" => "arn:aws:iam::187416307283:role/service-role/AWSBackupDefaultServiceRole" name: "tf_acc_test_backup_selection_5994421555613158377" => "tf_acc_test_backup_selection_5994421555613158377" plan_id: "7e69f127-aab4-4217-90cc-437ec8d9e19a" => "7e69f127-aab4-4217-90cc-437ec8d9e19a" resources.#: "2" => "2" resources.0: "arn:aws:ec2:us-west-2:123456789012:volume/" => "arn:aws:elasticfilesystem:us-west-2:123456789012:file-system/" (forces new resource) resources.1: "arn:aws:elasticfilesystem:us-west-2:123456789012:file-system/" => "arn:aws:ec2:us-west-2:123456789012:volume/" (forces new resource) selection_tag.#: "1" => "1" selection_tag.3487478888.key: "foo" => "foo" selection_tag.3487478888.type: "STRINGEQUALS" => "STRINGEQUALS" selection_tag.3487478888.value: "bar" => "bar"

So this can be switched to TypeSet 👍

bflad · 2019-04-05T10:49:01Z

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

ghost · 2020-03-30T17:51:25Z

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!

r/aws_backup_selection: Adding resource to manage selections for AWS …

859d751

…Backup plans

ewbankkit mentioned this pull request Jan 30, 2019

Support for AWS Backup #7166

Closed

slapula added 2 commits February 2, 2019 13:55

Merge branch 'resource-aws-backup-plan' into resource-aws-backup-sele…

9584ff2

…ction

adding tests

efb62f3

bflad added new-resource Introduces a new resource. service/backup Issues and PRs that pertain to the backup service. labels Feb 4, 2019

Merge branch 'master' into resource-aws-backup-selection

f6264b4

ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. labels Mar 18, 2019

cleaning up tests and error handling

c33b851

ewbankkit mentioned this pull request Mar 25, 2019

R/aws backup selection #8062

Closed

bflad reviewed Mar 29, 2019

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Mar 29, 2019

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Mar 29, 2019

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Mar 29, 2019

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Mar 29, 2019

slapula added 5 commits March 29, 2019 19:46

Merge branch 'master' into resource-aws-backup-selection

7884809

changes based on review feedback

33efb69

forgot to document the id output value

4d2e478

forgot to update all iam_role_arn references

8d4ee52

fixing resource tfstate issue

5132ea5

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Apr 1, 2019

more changes based on feedback

c80dd6f

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Apr 4, 2019

bflad added this to the v2.5.0 milestone Apr 4, 2019

bflad approved these changes Apr 4, 2019

View reviewed changes

bflad merged commit c80dd6f into hashicorp:master Apr 4, 2019

bflad added a commit that referenced this pull request Apr 4, 2019

Update CHANGELOG for #7382

0c73c96

grom3k mentioned this pull request Apr 5, 2019

Reflect actual argument names for aws_backup_selection in docs #8212

Merged

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r/aws_backup_selection: Adding resource to manage selections for AWS Backup plans #7382

r/aws_backup_selection: Adding resource to manage selections for AWS Backup plans #7382

slapula commented Jan 30, 2019 •

edited

Loading

bflad left a comment

bflad Mar 29, 2019

slapula Mar 30, 2019

bflad Apr 1, 2019 •

edited

Loading

slapula Apr 4, 2019

asweat-pubfactory commented Mar 29, 2019

amirgrois commented Mar 29, 2019

slapula commented Mar 29, 2019

slapula commented Mar 30, 2019

bflad commented Apr 1, 2019

slapula commented Apr 4, 2019

bflad left a comment

bflad Apr 4, 2019

bflad Apr 4, 2019

bflad commented Apr 5, 2019

ghost commented Mar 30, 2020

r/aws_backup_selection: Adding resource to manage selections for AWS Backup plans #7382

r/aws_backup_selection: Adding resource to manage selections for AWS Backup plans #7382

Conversation

slapula commented Jan 30, 2019 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

bflad Mar 29, 2019

Choose a reason for hiding this comment

slapula Mar 30, 2019

Choose a reason for hiding this comment

bflad Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

slapula Apr 4, 2019

Choose a reason for hiding this comment

asweat-pubfactory commented Mar 29, 2019

amirgrois commented Mar 29, 2019

slapula commented Mar 29, 2019

slapula commented Mar 30, 2019

bflad commented Apr 1, 2019

slapula commented Apr 4, 2019

bflad left a comment

Choose a reason for hiding this comment

bflad Apr 4, 2019

Choose a reason for hiding this comment

bflad Apr 4, 2019

Choose a reason for hiding this comment

bflad commented Apr 5, 2019

ghost commented Mar 30, 2020

slapula commented Jan 30, 2019 •

edited

Loading

bflad Apr 1, 2019 •

edited

Loading