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

New Data Source: aws_cloudformation_exports #2180

Merged
merged 2 commits into from
Jun 15, 2018

Conversation

moofish32
Copy link
Contributor

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.

@radeksimko radeksimko added the new-data-source Introduces a new data source. label Nov 7, 2017
@radeksimko radeksimko added the size/L Managed by automation to categorize the size of a PR. label Nov 15, 2017
@trung
Copy link
Contributor

trung commented Nov 29, 2017

This is straightforward implementation for https://docs.aws.amazon.com/AWSCloudFormation/latest/APIReference/API_ListExports.html

I would change stack_id to exporting_stack_id to match with AWS model

@trung
Copy link
Contributor

trung commented Dec 19, 2017

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

@trung
Copy link
Contributor

trung commented Jan 5, 2018

@radeksimko appreciate your review on this PR

@trung
Copy link
Contributor

trung commented Jan 12, 2018

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

@bflad bflad added the service/cloudformation Issues and PRs that pertain to the cloudformation service. label Jan 12, 2018
@radeksimko radeksimko changed the title Adding data source for Cloudformation Exports New Data Source: aws_cloudformation_exports Jan 17, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 11, 2018
@moofish32
Copy link
Contributor Author

@radeksimko -- rebase to resolve conflicts

@moofish32
Copy link
Contributor Author

@radeksimko @Ninir -- any update here?

@trung
Copy link
Contributor

trung commented Mar 18, 2018

@bflad can you have a quick look at this? Thanks

@trung
Copy link
Contributor

trung commented May 15, 2018

hope this can be merged soon so separate build just to include this PR is no longer needed

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label May 16, 2018
@moofish32
Copy link
Contributor Author

rebased to master @bflad @radeksimko not sure if there is something missing here?

@johnmcteague
Copy link

Any ETA on when this could get merged?

@paultyng
Copy link
Contributor

For the most part this looks ok to me, the only change I would make would be to rename it to aws_cloudformation_export as its just a single export value, to leave room if we want to introduce a data source that pulls multiple exports at once.

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Jun 15, 2018
@moofish32
Copy link
Contributor Author

renamed to cloudformatin_export

@bflad bflad added this to the v1.24.0 milestone Jun 15, 2018
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.

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

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

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

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

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

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

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

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

@bflad bflad merged commit 1f65313 into hashicorp:master Jun 15, 2018
bflad added a commit that referenced this pull request Jun 15, 2018
…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 added a commit that referenced this pull request Jun 15, 2018
@moofish32 moofish32 deleted the f-cfn-exports branch June 15, 2018 20:23
@moofish32
Copy link
Contributor Author

@bflad -- thanks for the feedback I'll try to keep this in mind for future work.

@bflad
Copy link
Contributor

bflad commented Jun 25, 2018

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.

@ghost
Copy link

ghost commented Apr 5, 2020

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 Apr 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new-data-source Introduces a new data source. service/cloudformation Issues and PRs that pertain to the cloudformation service. size/L Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants