-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
}, | ||
}, | ||
}, | ||
Set: resourceAwsConditionTagHash, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should simplify this attribute by removing the custom Set
function and allowing Terraform to use its default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :-)
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. 👍 |
waiting as well |
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. |
@bflad I believe I've addressed all of your comments:
|
From quick glance it looks like removing |
@bflad I went ahead and changed the attribute to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
} | ||
|
||
resources = [ | ||
"arn:aws:ec2:us-east-1:${data.aws_caller_identity.current.account_id}:volume/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The acceptance testing runs in us-west-2
by default:
--- 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/"
}, | ||
}, | ||
"resources": { | ||
Type: schema.TypeList, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
👍
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. |
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! |
Fixes #7166
Changes proposed in this pull request:
aws_backup_selection
Output from acceptance testing: