-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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_ram_resource_share: Add new resource #6528
Conversation
3861a25
to
9dafc45
Compare
Hey @gazoakley 👋 I'm guessing this is work in progress due to the acceptance testing? Anything we can help with? Since we now support EC2 Transit Gateway and License Manager Configurations, maybe we can use either of those for testing? Thanks so much for all your great contributions! |
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.
Two quick little drive-by comments since I know this is work in progress 👍
@@ -143,6 +144,7 @@ type Config struct { | |||
KinesisAnalyticsEndpoint string | |||
KmsEndpoint string | |||
LambdaEndpoint string | |||
RamEndpoint string |
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.
Since we're not wiring up endpoint customization in the provider configuration (see aws/provider.go
) we should either implement it or remove it here, e.g. removing this line, awsRamSess
and using client.ramconn = ram.New(sess)
👍
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.
This still needs to be addressed either which way. 😄
Hi @bflad - at the time I started writing this AWS hadn't announced what you could share using it 😄. I'm planning to write acceptance tests using VPCs since they can be constructed/torn down quickly during acceptance tests. What I've got kind of works, but I'm thinking it would be better to split The design then becomes something along the lines of: resource "aws_ram_resource_share" "example" {
name = "Example"
allow_external_principals = true
}
resource "aws_ram_association" "example" {
resource_share_arn = "${aws_ram_resource_share.example.arn}"
association_type = "PRINCIPAL"
associated_entity = "123456789012"
}
resource "aws_ram_association" "example" {
resource_share_arn = "${aws_ram_resource_share.example.arn}"
association_type = "RESOURCE"
associated_entity = "<some_vpc_arn>"
} Or better still in my mind: resource "aws_ram_resource_share" "example" {
name = "Example"
allow_external_principals = true
}
resource "aws_ram_principal_association" "example" {
resource_share_arn = "${aws_ram_resource_share.example.arn}"
principal = "123456789012"
# external - calculated field that is only applicable to principals associated to a resource share
}
resource "aws_ram_resource_association" "example" {
resource_share_arn = "${aws_ram_resource_share.example.arn}"
resource_arn = "<some_vpc_arn>"
} Any thoughts? |
@gazoakley looking at the API, I would agree with your assessment and the second approach with two associations resources. The time tracking is likely pretty important from an infrastructure management standpoint to allow a single Terraform configuration to perform downstream actions when the associations are actually ready. 👍 If you could split those new resources from this PR, we can likely get this resource merged then work on the association bits separately. 🚀 |
0b4647e
to
5ad9071
Compare
@bflad I've done the split - will open PRs for the associations later |
Unfortunately there's an issue in |
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.
Hey again, @gazoakley! For this one some minor stuff and it should be good to go! I'm not sure if we should implement the suggested Go SDK workaround with the error handling, but I think this is acceptable either which way in that regard. Thanks for all your hard work!
@@ -143,6 +144,7 @@ type Config struct { | |||
KinesisAnalyticsEndpoint string | |||
KmsEndpoint string | |||
LambdaEndpoint string | |||
RamEndpoint string |
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.
This still needs to be addressed either which way. 😄
return &schema.Resource{ | ||
Create: resourceAwsRamResourceShareCreate, | ||
Read: resourceAwsRamResourceShareRead, | ||
Update: resourceAwsRamResourceShareUpdate, |
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 add at least one acceptance test that exercises Update
😄
} | ||
|
||
stateConf := &resource.StateChangeConf{ | ||
Pending: []string{ram.ResourceShareStatusDeleting}, |
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 may also want to include ram.ResourceShareStatusActive
in the Pending
list in case there is any delay the start of the process. 👍
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.
LGTM, thanks @gazoakley! 🚀 FYI I addressed the remaining minor review items in a followup commit (1070d83) so we can get this released, I hope you don't mind.
Output from acceptance testing:
--- PASS: TestAccAwsRamResourceShare_basic (13.39s)
--- PASS: TestAccAwsRamResourceShare_Name (20.76s)
--- PASS: TestAccAwsRamResourceShare_AllowExternalPrincipals (21.53s)
--- PASS: TestAccAwsRamResourceShare_Tags (29.14s)
Changes: * Remove incomplete implementation for provider-level RAM endpoint configuration * Add covering acceptance testing for updates of all attributes Output from acceptance testing: ``` --- PASS: TestAccAwsRamResourceShare_basic (13.39s) --- PASS: TestAccAwsRamResourceShare_Name (20.76s) --- PASS: TestAccAwsRamResourceShare_AllowExternalPrincipals (21.53s) --- PASS: TestAccAwsRamResourceShare_Tags (29.14s) ```
This has been released in version 1.56.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
@bflad the Terraform documentation shows only about |
@piersf you can follow these for updates:
|
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! |
Partially addresses #6527
Output from acceptance testing: