data-source/aws_region: Remove EC2 API call, default to current if no endpoint or name specified #3157

bflad · 2018-01-27T05:22:49Z

This PR accomplishes a few things here:

Removes ec2.DescribeRegions API call. The SDK provides all regions and EC2 endpoint information already and this resource will automatically update when we update the SDK. The provider will not work in a new region without an updated release anyways.
Fixes current to match the documentation that listed it as optional, so current = true is no longer necessary if endpoint and name are omitted.

make testacc TEST=./aws TESTARGS='-run=TestAccDataSourceAwsRegion'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccDataSourceAwsRegion -timeout 120m
=== RUN   TestAccDataSourceAwsRegion_basic
--- PASS: TestAccDataSourceAwsRegion_basic (6.91s)
=== RUN   TestAccDataSourceAwsRegion_endpoint
--- PASS: TestAccDataSourceAwsRegion_endpoint (8.90s)
=== RUN   TestAccDataSourceAwsRegion_name
--- PASS: TestAccDataSourceAwsRegion_name (10.58s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	26.443s

… endpoint or name

radeksimko

Just a few nitpicky questions. I appreciate this is a first step of the refactoring and already very helpful (getting rid of extra API call), but generally I'd be probably even a bit more aggressive and deprecated current.

It's no big deal though. We can do it in a separate PR.

radeksimko · 2018-02-09T11:30:57Z

website/docs/d/region.html.markdown

@@ -35,12 +33,9 @@ exported as attributes.
 * `name` - (Optional) The full name of the region to select.

 * `current` - (Optional) Set to `true` to match only the region configured
-  in the provider. (It is not meaningful to set this to `false`.)
+  in the provider. Defaults to `true` if `endpoint` or `name` is not given.


I'm curious what is the value of current = true still. 🤔 Can't we just assume it's always true in such conditions and deprecate the field?

radeksimko · 2018-02-09T11:31:11Z

website/docs/d/region.html.markdown

@@ -51,4 +46,4 @@ The following attributes are exported:
 * `current` - `true` if the selected region is the one configured on the
  provider, or `false` otherwise.

-* `endpoint` - The endpoint for the selected region.
+* `endpoint` - The EC2 endpoint for the selected region.


Thank you! 👍

radeksimko · 2018-02-09T11:33:45Z

aws/data_source_aws_region.go

+					break
+				}
+				if current && (*matchingRegion).ID() != currentRegion {
+					return fmt.Errorf("multiple regions matched; use additional constraints to reduce matches to a single region")


Is there any reason we can't just error out here regardless of current?

Ha, I think I was trying to make it so it could support current being false or something. I'm on board with deprecating the current attribute completely (making it have no effect in the interim period) so this extra logic is moot and I'll remove it now.

radeksimko · 2018-02-09T11:37:31Z

aws/data_source_aws_region.go

+		}
+		if matchingRegion == nil {
+			return fmt.Errorf("region not found for endpoint: %s", endpoint)
+		}


It took me a while to understand what this block of code does. 😅 Would you mind extracting it into something like matchingRegion, err := findRegionByEndpoint(endpoint)?

That's a great idea! Let me implement that.

radeksimko · 2018-02-09T11:38:04Z

aws/data_source_aws_region.go

+		}
+		if matchingRegion == nil {
+			return fmt.Errorf("region not found for name: %s", name)
+		}


Likewise - how do you feel about extracting this into matchingRegion, err := findRegionByName(name)?

radeksimko · 2018-02-09T11:39:19Z

aws/data_source_aws_region.go

-	}
-
-	region := resp.Regions[0]
+	d.Set("endpoint", strings.TrimPrefix(regionEndpointEc2.URL, "https://"))


Just out of curiosity: What is the reason for the trimming here?

Previously, the EC2 API response was returning just the hostname (see example here: https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_DescribeRegions.html), however the SDK ResolvedEndpoint.URL prepends https:// to the front. We weren't previously testing the old behavior, but the acceptance testing shows it doing the required trimming (I think).

…nd findRegionByName functions

bflad · 2018-02-09T14:50:20Z

@radeksimko refactored those two functions out, much cleaner. 😄

make testacc TEST=./aws TESTARGS='-run=TestAccDataSourceAwsRegion'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccDataSourceAwsRegion -timeout 120m
=== RUN   TestAccDataSourceAwsRegion_basic
--- PASS: TestAccDataSourceAwsRegion_basic (6.37s)
=== RUN   TestAccDataSourceAwsRegion_endpoint
--- PASS: TestAccDataSourceAwsRegion_endpoint (5.74s)
=== RUN   TestAccDataSourceAwsRegion_endpointAndName
--- PASS: TestAccDataSourceAwsRegion_endpointAndName (9.67s)
=== RUN   TestAccDataSourceAwsRegion_name
--- PASS: TestAccDataSourceAwsRegion_name (8.01s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	29.817s

make test TEST=./aws
==> Checking that code complies with gofmt requirements...
go test ./aws -timeout=30s -parallel=4
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1.695s

bflad · 2018-02-09T22:20:22Z

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.

* commit '5293a0e3b1366ee16d8742b9b2354781a79bfbd9': (224 commits) v1.9.0 Update CHANGELOG for hashicorp#1101 and hashicorp#3283 docs/resource/aws_sns_platform_application: Add note about platform_credential and platform_principal hashing resource/aws_sns_platform_application: Refactor ID parsing to its own function, use testing.T in testAccAwsSnsPlatformApplicationPlatformFromEnv Add lambda example (hashicorp#3168) Update CHANGELOG for hashicorp#3157 docs/data-source/aws_region: Remove now deprecated current argument data-source/aws_region: Refactor logic into findRegionByEc2Endpoint and findRegionByName functions Update CHANGELOG for hashicorp#3301 Update CHANGELOG for hashicorp#2559 and hashicorp#3240 Update CHANGELOG.md resource/aws_kinesis_stream: Retry deletion on LimitExceededException (hashicorp#3108) Update CHANGELOG.md resource/aws_dynamodb_table_item: Cleanup + add missing bits Added dynamodb_table_item resource hashicorp#517 Update CHANGELOG.md New Resource: aws_cloud9_environment_ec2 Update CHANGELOG.md Fixed markdown typo in docs resource/aws_kinesis_firehose_delivery_stream: Prevent crashes on empty CloudWatchLoggingOptions and fix extended_s3_configuration kms_key_arn ... # Conflicts: # aws/validators.go

…parameters-features * commit '5293a0e3b1366ee16d8742b9b2354781a79bfbd9': (752 commits) v1.9.0 Update CHANGELOG for hashicorp#1101 and hashicorp#3283 docs/resource/aws_sns_platform_application: Add note about platform_credential and platform_principal hashing resource/aws_sns_platform_application: Refactor ID parsing to its own function, use testing.T in testAccAwsSnsPlatformApplicationPlatformFromEnv Add lambda example (hashicorp#3168) Update CHANGELOG for hashicorp#3157 docs/data-source/aws_region: Remove now deprecated current argument data-source/aws_region: Refactor logic into findRegionByEc2Endpoint and findRegionByName functions Update CHANGELOG for hashicorp#3301 Update CHANGELOG for hashicorp#2559 and hashicorp#3240 Update CHANGELOG.md resource/aws_kinesis_stream: Retry deletion on LimitExceededException (hashicorp#3108) Update CHANGELOG.md resource/aws_dynamodb_table_item: Cleanup + add missing bits Added dynamodb_table_item resource hashicorp#517 Update CHANGELOG.md New Resource: aws_cloud9_environment_ec2 Update CHANGELOG.md Fixed markdown typo in docs resource/aws_kinesis_firehose_delivery_stream: Prevent crashes on empty CloudWatchLoggingOptions and fix extended_s3_configuration kms_key_arn ... # Conflicts: # aws/resource_aws_ssm_parameter_test.go

removing current will fix the messages. Warning: module.platform.module.ecs.data.aws_region.current: "current": [DEPRECATED] Defaults to current provider region if no other filtering is enabled You can find more info at hashicorp/terraform-provider-aws#3157

ghost · 2020-04-06T17:15:22Z

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!

data-source/aws_region: Remove EC2 API call, default to current if no…

904e61f

… endpoint or name

ghost added the size/L Managed by automation to categorize the size of a PR. label Jan 27, 2018

bflad added enhancement Requests to existing resources that expand the functionality or scope. provider Pertains to the provider itself, rather than any interaction with AWS. labels Jan 27, 2018

radeksimko approved these changes Feb 9, 2018

View reviewed changes

data-source/aws_region: Refactor logic into findRegionByEc2Endpoint a…

c681f74

…nd findRegionByName functions

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 9, 2018

docs/data-source/aws_region: Remove now deprecated current argument

1cc81c9

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 9, 2018

bflad merged commit 2f001b5 into master Feb 9, 2018

bflad deleted the f-aws_region-no-api-current-default branch February 9, 2018 15:00

bflad added this to the v1.9.0 milestone Feb 9, 2018

bflad added a commit that referenced this pull request Feb 9, 2018

Update CHANGELOG for #3157

c189b77

disaac mentioned this pull request Feb 21, 2018

Remove aws region current boolean Deprecated in AWS provider unifio/terraform-aws-apigw#9

Closed

jasonsattler mentioned this pull request Mar 2, 2018

Fix the DEPRECATED warnings WasabiAiR/terraform-aws-platform#35

Merged

ghost locked and limited conversation to collaborators Apr 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

data-source/aws_region: Remove EC2 API call, default to current if no endpoint or name specified #3157

data-source/aws_region: Remove EC2 API call, default to current if no endpoint or name specified #3157

bflad commented Jan 27, 2018

radeksimko left a comment

radeksimko Feb 9, 2018

radeksimko Feb 9, 2018

radeksimko Feb 9, 2018

bflad Feb 9, 2018

radeksimko Feb 9, 2018

bflad Feb 9, 2018

radeksimko Feb 9, 2018

bflad Feb 9, 2018

radeksimko Feb 9, 2018

bflad Feb 9, 2018

bflad commented Feb 9, 2018

bflad commented Feb 9, 2018

ghost commented Apr 6, 2020

data-source/aws_region: Remove EC2 API call, default to current if no endpoint or name specified #3157

data-source/aws_region: Remove EC2 API call, default to current if no endpoint or name specified #3157

Conversation

bflad commented Jan 27, 2018

radeksimko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Feb 9, 2018

bflad commented Feb 9, 2018

ghost commented Apr 6, 2020