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

d/elastic_beanstalk_hosted_zones - new data source #3208

Merged
merged 5 commits into from
Feb 5, 2018

Conversation

sjauld
Copy link
Contributor

@sjauld sjauld commented Jan 31, 2018

Ref #2952

I know the above issue was closed, but I think it makes sense for this mapping of elasticbeanstalk zones to be here as a resource rather than everyone maintaining their own sets of maps.

The big update to aws/provider.go is from go fmt - not sure if it was deliberately like this and I should revert (my editor go fmts automatically)?

TF_ACC=1 aws-vault exec development -- go test -v -run TestAccAwsDataSourceElasticBeanstalkHostedZone ./...
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsDataSourceElasticBeanstalkHostedZone
--- PASS: TestAccAwsDataSourceElasticBeanstalkHostedZone (50.47s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	50.531s

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Jan 31, 2018
@radeksimko radeksimko added service/elasticbeanstalk Issues and PRs that pertain to the elasticbeanstalk service. new-data-source Introduces a new data source. labels Jan 31, 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.

@sjauld I think this is a valid addition for the reason you mentioned. We want this provider to be helpful even when AWS isn't (they should really provide these easier than a documentation webpage 😉 ). We already maintain these lists for other IDs like ELB Hosted Zone IDs so the burden of maintenance is not that much higher.

So with that said, thanks for the contribution! I left some mostly minor notes below. We should definitely add a line item to the New Regions section of CONTRIBUTING.md for maintainers or others to help add support for new regions when they do come online.

"github.com/hashicorp/terraform/helper/schema"
)

const unsupportedElasticBeanstalkRegion = "UnsupportedRegion"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can skip something like this. 😄 I'll explain below

const unsupportedElasticBeanstalkRegion = "UnsupportedRegion"

// See # http://docs.aws.amazon.com/general/latest/gr/rande.html#elasticbeanstalk_region
var elasticBeanstalkHostedZoneIds = map[string]string{
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: can this map be arranged alphabetically please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do. My approach was based on mirroring the ordering in the AWS docs, but given the low-frequency at which updates will be required I prefer alphabetical too.

region = v.(string)
}

zoneID := elasticBeanstalkHostedZoneIds[region]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we appreciate the distinction between unknown and unsupported regions at the current moment this is being developed, I think we should simplify this so we do not need to maintain the difference in the future. Recently we switched the provider so AWS SDK updates with new regions will automatically validate them. We can add manually checking the Regions and Endpoints documentation for this EB Hosted Zone data source to the CONTRIBUTING.md New Regions section of our documentation.

zoneID, ok := elasticBeanstalkHostedZoneIds[region]
if !ok {
  return fmt.Errorf("Unsupported region: %s", region)
}

Providers: testAccProviders,
Steps: []resource.TestStep{
{
Config: testAccCheckAwsElasticBeanstalkHostedZoneDataSource_currentRegion,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a check for an unsupported region please?

{
  Config: testAccCheckAwsElasticBeanstalkHostedZoneDataSource_region("does-not-exist"),
  ExpectError: regexp.MustCompile(`Unsupported region`),
},


# Data Source: aws_elastic_beanstalk_hosted_zone

Use this data source to get the ID of an elastic beanstalk hosted zone.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to use the link at the bottom? If not, could you use it please? 😄

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 5, 2018
@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 5, 2018
@sjauld
Copy link
Contributor Author

sjauld commented Feb 5, 2018

Done!

TF_ACC=1 aws-vault exec development -- go test -v -run TestAccAwsDataSourceElasticBeanstalkHostedZone ./...
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsDataSourceElasticBeanstalkHostedZone
--- PASS: TestAccAwsDataSourceElasticBeanstalkHostedZone (59.31s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	59.346s

@bflad bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Feb 5, 2018
@bflad bflad self-requested a review February 5, 2018 21:38
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.

Thanks @sjauld! This looks great! 🚀

make testacc TEST=./aws TESTARGS='-run=TestAccAwsDataSourceElasticBeanstalkHostedZone'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAwsDataSourceElasticBeanstalkHostedZone -timeout 120m
=== RUN   TestAccAwsDataSourceElasticBeanstalkHostedZone
--- PASS: TestAccAwsDataSourceElasticBeanstalkHostedZone (19.66s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	19.716s

@bflad bflad added this to the v1.9.0 milestone Feb 5, 2018
@bflad bflad merged commit 65ef3dd into hashicorp:master Feb 5, 2018
@sjauld sjauld deleted the d/aws_eb_zones branch February 5, 2018 21:45
bflad added a commit that referenced this pull request Feb 5, 2018
@bflad
Copy link
Contributor

bflad commented Feb 9, 2018

This has been released in terraform-provider-aws version 1.9.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Apr 8, 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 8, 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/elasticbeanstalk Issues and PRs that pertain to the elasticbeanstalk 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.

3 participants