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

Support AWS cn-northwest-1 Ningxia (fixes #3053) #3142

Merged
merged 3 commits into from
Jan 26, 2018

Conversation

kwerey
Copy link
Contributor

@kwerey kwerey commented Jan 26, 2018

Hi Terraform team - this adds the new Ningxia region to the list of valid options for the AWS provider, fixing #3053

A lot of the acceptance tests will fail run in Nginxia, since the region doesn't support a very large number of AWS services, but these changes let the provider can successfully make client connections to the region:

$ export AWS_DEFAULT_REGION=cn-northwest-1
$ make testacc TEST=.//aws TESTARGS='-run=TestAccAWSCloudWatchMetricAlarm_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test .//aws -v -run=TestAccAWSCloudWatchMetricAlarm_basic -timeout 120m
=== RUN   TestAccAWSCloudWatchMetricAlarm_basic
--- PASS: TestAccAWSCloudWatchMetricAlarm_basic (55.58s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	56.088s

I've not read a single line of go source code before today, so please let me know if there's a neater way to add the region. Is there an "is in [list of regions]" comparison operator for strings? I didn't see one with a cursory google!

Thanks for making a cool project, I've spent a few days prototyping stuff in TF and I like it lot 👍

@bflad bflad self-requested a review January 26, 2018 12:59
@bflad bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Jan 26, 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.

Hi @kwerey!

Thanks for the kind words and diving into this! We do not have fully written documentation around adding a region yet since it happens infrequently, but here are some additional items to make this PR ready to go. These are mostly around getting new AWS account IDs for services that are currently published. If you do not have time for the additional work, please let us know and we can finish this up for you keeping your existing commit.

  • Add "cn-northwest-1": "", // Not supported as of January 2018 to aws/data_source_aws_elb_hosted_zone_id.go
  • Add "cn-northwest-1": "", // Not supported as of January 2018 to aws/hosted_zones.go
  • Add "cn-northwest-1": "681348832753", to aws/data_source_aws_cloudtrail_service_account.go
  • Add "cn-northwest-1": "037604701340", to aws/data_source_aws_elb_service_account.go
  • Add "cn-northwest-1": "660998842044", to aws/data_source_aws_redshift_service_account.go

Thanks so much!
Brian

Maintainer's notes:

I do not believe we have written documentation on this yet, but using #2707 as a baseline, adding a new region is currently:

  1. Add region to aws/config.go (maybe can be handled automatically by the SDK endpoints helper functions: https://docs.aws.amazon.com/sdk-for-go/api/aws/endpoints/)
  2. Check Regions and Endpoints ELB regions and add Route53 Hosted Zone IDs to aws/data_source_aws_elb_hosted_zone_id.go
  3. Check Regions and Endpoints S3 website endpoints and add Route53 Hosted Zone ID to aws/hosted_zones.go
  4. Check CloudTrail Supported Regions docs and add AWS Account ID to aws/data_source_aws_cloudtrail_service_account.go
  5. Check Elastic Load Balancing Access Logs docs and add Elastic Load Balancing Account ID to aws/data_source_aws_elb_service_account.go
  6. Check Redshift Database Audit Logging AWS Account ID docs and add to aws/data_source_aws_redshift_service_account.go

@kwerey
Copy link
Contributor Author

kwerey commented Jan 26, 2018

No problem Brian, I've added those IDs.

@kwerey kwerey force-pushed the cn-northwest-1-3053 branch from e5c1f9a to ff22562 Compare January 26, 2018 14:18
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.

LGTM!

@bflad
Copy link
Contributor

bflad commented Jan 26, 2018

Sorry I realized I didn't specifically answer your question here:

Is there an "is in [list of regions]" comparison operator for strings? I didn't see one with a cursory google!

The endpoints package in the AWS Go SDK does have constants and some helper functions which we could utilize to automate this region/partition handling/validation for us. We would greatly appreciate any pull requests that utilized either the region constants across the codebase or the helper functions. 😄

@bflad bflad added this to the v1.8.0 milestone Jan 26, 2018
@bflad
Copy link
Contributor

bflad commented Jan 26, 2018

Thanks for removing those empty mappings (that was a bad recommendation on my part). I'm going to merge this in so this new region can potentially be used starting with the v1.8.0 release.

@bflad bflad merged commit bf7787d into hashicorp:master Jan 26, 2018
@kwerey
Copy link
Contributor Author

kwerey commented Jan 26, 2018

No problem, I read the thread on that docs PR and went "yup, that's legit".

I'd love to see cloud integration tools move away from having to keep all these lists in their own codebases - if we end up committing to TF at my shop I'll try to get some work in on it.

@kwerey kwerey deleted the cn-northwest-1-3053 branch January 26, 2018 16:39
bflad added a commit that referenced this pull request Jan 26, 2018
@kwerey
Copy link
Contributor Author

kwerey commented Jan 26, 2018

Sweet, I get 'contributor' by my name! \o/ Thanks for getting feedback in on this PR so quick, it's really neat to be able to just jump in and get something useful sorted out in a project in a matter of hours.

@bflad
Copy link
Contributor

bflad commented Jan 29, 2018

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

Important note: if you're planning on using cn-northwest-1 S3 for backend state, this PR (or newer version of it) needs to be merged and released in Terraform core (which will be 0.11.3+): hashicorp/terraform#17216

jocgir added a commit to coveooss/terraform-provider-aws that referenced this pull request Jan 31, 2018
* commit '8937a3a4e9d77c8089cf147861b604e3a2d8cf7e': (47 commits)
  v1.8.0
  Update CHANGELOG.md
  resource/aws_dynamodb_table: Refactoring (hashicorp#3136)
  Update CHANGELOG for hashicorp#3171
  resource/aws_elastic_beanstalk_application: Prevent crash on reading missing application
  chore(vendor): bump aws-sdk-go to v1.12.70
  typo guardduty import test
  Update CHANGELOG for hashicorp#3142
  Removed empty strings
  test/aws_dynamodb_global_table: Use single region for basic and import testing
  resource/aws_sqs_queue: Retry creation on QueueDeletedRecently for additional 10 seconds
  docs/CONTRIBUTING: Use aws_cloudwatch_dashboard for acceptance test examples
  docs/CONTRIBUTING: New Region: don't add empty mappings
  Update CHANGELOG.md
  resource/aws_rds_cluster: Retry deletion on InvalidDBClusterStateFault
  docs/CONTRIBUTING: Remove pre-split provider directory references from testacc commands
  Add service account IDs
  docs/CONTRIBUTING: New Region
  Support AWS cn-northwest-1 Ningxia (fixes hashicorp#3053)
  Update CHANGELOG.md
  ...

# Conflicts:
#	aws/validators.go
@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
enhancement Requests to existing resources that expand the functionality or scope.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants