New Resource: r/aws_config_authorization #4263

gazoakley · 2018-04-19T10:26:38Z

Partially implements #4067

New Resources:

aws_config_authorization

$ AWS_PROFILE=personal make testacc TESTARGS='-run=TestAccConfigAuthorization'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccConfigAuthorization -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccConfigAuthorization_import
--- PASS: TestAccConfigAuthorization_import (24.76s)
=== RUN   TestAccConfigAuthorization_basic
--- PASS: TestAccConfigAuthorization_basic (21.88s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	46.688s

bflad

Thanks for submitting this @gazoakley! 👍 Overall the PR is really good and passes acceptance testing -- there are just a few minor things that I will spend time adjusting post-merge for naming clarity, bug prevention, and to adjust this PR for some forward looking initiatives we have in the provider.

bflad · 2018-06-03T13:13:58Z

aws/resource_aws_config_authorization.go

+		return fmt.Errorf("Error deleting authorization: %s", err)
+	}
+
+	d.SetId("")


Minor nitpick: d.SetId("") is extraneous in delete functions

bflad · 2018-06-03T13:17:35Z

aws/provider.go

@@ -303,6 +303,7 @@ func Provider() terraform.ResourceProvider {
 			"aws_cloudwatch_log_resource_policy":           resourceAwsCloudWatchLogResourcePolicy(),
 			"aws_cloudwatch_log_stream":                    resourceAwsCloudWatchLogStream(),
 			"aws_cloudwatch_log_subscription_filter":       resourceAwsCloudwatchLogSubscriptionFilter(),
+			"aws_config_authorization":                     resourceAwsConfigAuthorization(),


Sorry this is going to be very pedantic but the Config API refers to these as "aggregate" authorization and the resource/function naming should reflect that for clarity and in case they introduce other forms/types of authorization.

"aws_config_aggregate_authorization": resourceAwsConfigAggregateAuthoriation(),

bflad · 2018-06-03T13:21:30Z

aws/resource_aws_config_authorization.go

+	d.Set("account_id", accountId)
+	d.Set("region", region)
+
+	res, err := conn.DescribeAggregationAuthorizations(&configservice.DescribeAggregationAuthorizationsInput{})


This call does not currently handle NextToken so it can incorrectly report errors/missing resources in large or separate API responses.

bflad · 2018-06-03T13:22:28Z

aws/resource_aws_config_authorization_test.go

+	}
+	conn := client.(*AWSClient).configconn
+
+	resp, err := conn.DescribeAggregationAuthorizations(&configservice.DescribeAggregationAuthorizationsInput{})


This call does not currently handle NextToken so it can potentially miss authorizations in large or separate API responses.

bflad · 2018-06-03T13:24:57Z

aws/resource_aws_config_authorization_test.go

+		resp, err := conn.DescribeAggregationAuthorizations(&configservice.DescribeAggregationAuthorizationsInput{})
+
+		if err == nil {
+			if len(resp.AggregationAuthorizations) != 0 &&


There may be additional other authorizations present, especially with testing concurrency or in non-testing accounts. This needs to be updated to specifically handle NextToken and loop through all authorizations, only returning an error if the account ID and region is found.

bflad · 2018-06-03T13:30:11Z

website/docs/r/config_authorization.markdown

+
+## Attributes Reference
+
+The following attributes are exported:


We're in the process of updating all these for clarity (#4685): In addition to all arguments above, the following attributes are exported:

bflad · 2018-06-03T13:32:29Z

aws/import_aws_config_authorization_test.go

+	"github.com/hashicorp/terraform/helper/resource"
+)
+
+func TestAccConfigAuthorization_import(t *testing.T) {


Minor nitpick: we're going to move away from separate import test files and tests (#4705). We can simply add the final TestStep the existing _basic test 👍

=== RUN TestAccAWSConfigAggregateAuthorization_basic --- PASS: TestAccAWSConfigAggregateAuthorization_basic (41.80s)

gazoakley · 2018-06-03T14:59:39Z

@bflad Could we have at least stuck with aws_config_aggregator_authorization? Currently after #4262 there'd be:

aws_config_aggregate_authorization
aws_config_aggregator

That doesn't gel well together - it suggests they might be related but not for certain (the console always refers to aggregator(s))

bflad · 2018-06-05T22:16:03Z

@gazoakley sorry for any initial confusion -- we wound up renaming both of the resources to match the API more closely. If the link between the two is not obvious we should probably cross-link them accordingly in the resource documentation for each of them.

This has been released in version 1.22.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-05T17:39:12Z

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 added the size/L Managed by automation to categorize the size of a PR. label Apr 19, 2018

bflad added new-resource Introduces a new resource. service/configservice Issues and PRs that pertain to the configservice service. labels Apr 20, 2018

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

gazoakley force-pushed the f-config-authorization branch from 9b2da85 to f48e11e Compare April 21, 2018 20:51

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

r/aws_config_authorization: New resource

41ce326

gazoakley force-pushed the f-config-authorization branch from f48e11e to 41ce326 Compare April 23, 2018 10:58

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

gazoakley changed the title ~~[WIP] New Resource: r/aws_config_authorization~~ New Resource: r/aws_config_authorization Apr 23, 2018

bflad approved these changes Jun 3, 2018

View reviewed changes

bflad added this to the v1.22.0 milestone Jun 3, 2018

bflad merged commit 41ce326 into hashicorp:master Jun 3, 2018

bflad added a commit that referenced this pull request Jun 3, 2018

resource/aws_config_aggregate_authorization: Implement PR #4263 feedback

1ab64a1

=== RUN TestAccAWSConfigAggregateAuthorization_basic --- PASS: TestAccAWSConfigAggregateAuthorization_basic (41.80s)

bflad added a commit that referenced this pull request Jun 3, 2018

Update CHANGELOG for #4263

2286bfc

bflad mentioned this pull request Jun 3, 2018

New Resource: r/aws_config_aggregator #4262

Merged

ghost locked and limited conversation to collaborators Apr 5, 2020

gazoakley deleted the f-config-authorization branch April 7, 2020 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: r/aws_config_authorization #4263

New Resource: r/aws_config_authorization #4263

gazoakley commented Apr 19, 2018 •

edited

Loading

bflad left a comment

bflad Jun 3, 2018

bflad Jun 3, 2018

bflad Jun 3, 2018

bflad Jun 3, 2018

bflad Jun 3, 2018

bflad Jun 3, 2018

bflad Jun 3, 2018

gazoakley commented Jun 3, 2018

bflad commented Jun 5, 2018

ghost commented Apr 5, 2020


		## Attributes Reference

		The following attributes are exported:

New Resource: r/aws_config_authorization #4263

New Resource: r/aws_config_authorization #4263

Conversation

gazoakley commented Apr 19, 2018 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

bflad Jun 3, 2018

Choose a reason for hiding this comment

bflad Jun 3, 2018

Choose a reason for hiding this comment

bflad Jun 3, 2018

Choose a reason for hiding this comment

bflad Jun 3, 2018

Choose a reason for hiding this comment

bflad Jun 3, 2018

Choose a reason for hiding this comment

bflad Jun 3, 2018

Choose a reason for hiding this comment

bflad Jun 3, 2018

Choose a reason for hiding this comment

gazoakley commented Jun 3, 2018

bflad commented Jun 5, 2018

ghost commented Apr 5, 2020

gazoakley commented Apr 19, 2018 •

edited

Loading