-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
New Data Source: aws_cloudformation_exports #2180
Conversation
This is straightforward implementation for https://docs.aws.amazon.com/AWSCloudFormation/latest/APIReference/API_ListExports.html I would change |
Is there any chance to have this in the coming 1.6.1 release? This helps a lot in an environment where the transition happening from CloudFormation-based approach to Terraform |
7c49f15
to
54b8996
Compare
@radeksimko appreciate your review on this PR |
@Ninir is there any chance you can look at this PR? I merged to my own fork and have been using it for awhile now. It works nicely. |
54b8996
to
459cdee
Compare
@radeksimko -- rebase to resolve conflicts |
@radeksimko @Ninir -- any update here? |
@bflad can you have a quick look at this? Thanks |
hope this can be merged soon so separate build just to include this PR is no longer needed |
rebased to master @bflad @radeksimko not sure if there is something missing here? |
Any ETA on when this could get merged? |
For the most part this looks ok to me, the only change I would make would be to rename it to |
renamed to cloudformatin_export |
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.
Sorry for the lengthy delay in getting this reviewed! 😅 As @paultyng mentioned this was in pretty good shape. I left some pretty minor feedback below, which I'll happily handle in a quick commit after yours. Thank you so much! 🚀
=== RUN TestAccAWSCloudformationExports_dataSource
--- PASS: TestAccAWSCloudformationExports_dataSource (58.17s)
func dataSourceAwsCloudFormationExportRead(d *schema.ResourceData, meta interface{}) error { | ||
conn := meta.(*AWSClient).cfconn | ||
var name, value string | ||
if v, ok := d.GetOk("name"); ok { |
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.
Minor nitpick: Since name
is required, we do not need to check for its existence with d.GetOk()
and instead can just get its value via name := d.Get("name").(string)
err := conn.ListExportsPages(input, | ||
func(page *cloudformation.ListExportsOutput, lastPage bool) bool { | ||
for _, e := range page.Exports { | ||
if name == *e.Name { |
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.
To prevent potential panics with the resource, we should use the SDK provided helpers to dereference the pointers rather than *
, e.g. aws.StringValue(e.Name)
return false | ||
} | ||
} | ||
if page.NextToken != nil { |
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 SDK paginators should automatically handle NextToken
so we can instead just return !lastPage
}) | ||
} | ||
|
||
const testAccCheckCfnExport = ` |
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.
😎 Extraneous constant here
--- | ||
layout: "aws" | ||
page_title: "AWS: aws_cloudformation_export" | ||
sidebar_current: "docs-aws-datasource-cloudformation-exports" |
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 filename and the sidebar naming need to be updated to remove the plural s
👍
Provides metadata of a CloudFormation Exports (e.g. Cross Stack References) | ||
--- | ||
|
||
# aws\_cloudformation\_exports |
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.
Minor nitpick: the forward slashes are no longer required in the documentation headers -- understandably this PR was written during a time either when they were required or many of the other documentation pages still had them
"github.com/hashicorp/terraform/helper/resource" | ||
) | ||
|
||
func TestAccAWSCloudformationExports_dataSource(t *testing.T) { |
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.
Plural s
is still present in this function name
…randomize test CF stack naming make testacc TEST=./aws TESTARGS='-run=TestAccAWSCloudformationExportDataSource_basic' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -run=TestAccAWSCloudformationExportDataSource_basic -timeout 120m === RUN TestAccAWSCloudformationExportDataSource_basic --- PASS: TestAccAWSCloudformationExportDataSource_basic (70.16s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 70.195s
@bflad -- thanks for the feedback I'll try to keep this in mind for future work. |
This has been released in version 1.24.0 of the 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! |
This adds a new data source to leverage Cloudformation Exports. The improvement offered by this feature is improved integration to Cloudformation Exports to ease the transition to Terraform. This will enable a team to directly reference the Export name instead of having to know the Cloudformation Stack name. With the use of nested stacks the stack name is often non-deterministic using post-pended random characters.
This was originally opened as hashicorp/terraform#14407, but I have changed the solution to use one export per resource instead of entire map. This creates a declarative solution allowing validation prior to run of the export values.