New Resource: r/aws_config_aggregator #4262

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

Partially implements #4067

New Resources:

aws_config_aggregator

$ make testacc TESTARGS='-run=TestAccConfigAggregator'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccConfigAggregator -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccConfigAggregator_import
--- PASS: TestAccConfigAggregator_import (25.42s)
=== RUN   TestAccConfigAggregator_account
--- PASS: TestAccConfigAggregator_account (28.26s)
=== RUN   TestAccConfigAggregator_organization
--- PASS: TestAccConfigAggregator_organization (28.64s)
=== RUN   TestAccConfigAggregator_switch
--- PASS: TestAccConfigAggregator_switch (44.97s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	127.344s

djk · 2018-05-22T12:09:00Z

Any sign of this being merged? 4263 as well?

gazoakley · 2018-05-22T13:55:13Z

@djk can you thumbs up the original post here please: #4067 (I think they use thumbs to prioritise reviews)

djk · 2018-05-22T14:05:49Z

@gazoakley Done, cheers!

bflad

This looks really good @gazoakley! Passes acceptance testing in our special Organizations testing account. 🚀 There are a few minor things similar to #4263 which I'll adjust post-merge that are noted below. Thanks!

bflad · 2018-06-03T13:41:53Z

aws/import_aws_config_aggregator_test.go

+	"github.com/hashicorp/terraform/helper/resource"
+)
+
+func TestAccConfigAggregator_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 👍

bflad · 2018-06-03T13:43:37Z

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_aggregator":                        resourceAwsConfigAggregator(),


Similar to #4263 we should prefer to clarify that this is a "configuration" aggregator to match the API, especially if they later introduce some other type of aggregator:

"aws_config_configuration_aggregator": resourceAwsConfigConfigurationAggregator(),

bflad · 2018-06-03T13:45:05Z

aws/resource_aws_config_aggregator.go

+		},
+
+		CustomizeDiff: customdiff.Sequence(
+			customdiff.ForceNewIfChange("account_aggregation_source", func(old, new, meta interface{}) bool {


I believe we can just set ForceNew: true on the root attributes instead of implementing CustomizeDiff here.

I was trying to minimise delete/recreate operations - it's OK to call PutConfigurationAggregatorInput to make changes to an existing aggregator as long as you aren't switching between organization or account aggregation sources.

Ah: its because of this behavior (seemingly buggy as ForceNew: doesn't necessarily apply to all nested attributes anymore):

All fields are ForceNew or Computed w/out Optional, Update is superfluous

I'll add this in a comment in there.

bflad · 2018-06-03T13:47:25Z

aws/resource_aws_config_aggregator.go

+		return err
+	}
+
+	if len(res.ConfigurationAggregators) == 0 {


While trying to prevent panics here, we should check res == nil as well 👍

bflad · 2018-06-03T13:50:19Z

aws/resource_aws_config_aggregator.go

+	d.Set("name", aggregator.ConfigurationAggregatorName)
+
+	if aggregator.AccountAggregationSources != nil {
+		d.Set("account_aggregation_source", flattenConfigAccountAggregationSources(aggregator.AccountAggregationSources))


Two notes:

The flatten function should handle nil input and return a zero length slice so we do not need to wrap this with a conditional

When setting non-scalar attributes in the Terraform state, we prefer to perform error checking:

if err := d.Set("account_aggregation_source", flattenConfigAccountAggregationSources(aggregator.AccountAggregationSources)); err != nil { return fmt.Errorf("error setting account_aggregation_source: %s", err) }

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

aws/resource_aws_config_aggregator.go

+		return err
+	}
+
+	d.SetId("")


d.SetId("") is extraneous in delete functions

bflad · 2018-06-03T13:53:49Z

aws/resource_aws_config_aggregator_test.go

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


This call is not currently handling NextToken so it can miss resources in large or separate API responses.

bflad · 2018-06-03T13:56:56Z

aws/structure.go

+	var result []interface{}
+	m := make(map[string]interface{})
+	m["account_ids"] = flattenStringList(source.AccountIds)
+	m["all_regions"] = *source.AllAwsRegions


To prevent potential panics we should prefer to use the SDK functions for dereferencing values here and below, e.g. aws.BoolValue(source.AllAwsRegions)

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

website/docs/r/config_aggregator.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-03T15:02:44Z

aws/resource_aws_config_aggregator_test.go

+
+  name = "tf-%s"
+
+  organization_aggregation_source {


MAINTAINERS NOTE: This will currently create an AWS Organizations organization, enable service control policy on it, and leave it dangling after the test. Will need to add the Terraform aws_organizations_organization resource to this test configuration and serialize with the rest of the AWS Organizations testing.

…back === RUN TestAccConfigAggregator_account --- PASS: TestAccConfigAggregator_account (36.87s) === RUN TestAccConfigAggregator_organization --- PASS: TestAccConfigAggregator_organization (39.71s) === RUN TestAccConfigAggregator_switch --- PASS: TestAccConfigAggregator_switch (59.08s)

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

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.

msvechla · 2019-04-08T14:42:55Z

Somehow the aws_config_configuration_aggregator is not persisted in terraform state for me. Terraform creates this ressource during every run.

Can somebody else verify this?

EDIT: Looks like a state import worked.

guipaivanz · 2019-04-17T04:18:12Z

Same problem here. Terraform apply keeps creating the resource but doesn't add to the state file.
After running terraform import it worked

guipaivanz · 2019-04-17T04:19:09Z

@bflad I believe there is a bug in terraform as the aws_config_configuration_aggregator is not storing the details in the state file

ghost · 2020-03-30T17:39:17Z

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

gazoakley force-pushed the f-config-aggregator branch from 30122c0 to d89a04f Compare April 22, 2018 09:27

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

gazoakley force-pushed the f-config-aggregator branch from d89a04f to 62833ba Compare April 22, 2018 10:45

ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/L Managed by automation to categorize the size of a PR. labels Apr 22, 2018

r/aws_config_aggregator: New resource

a3b5a6d

gazoakley force-pushed the f-config-aggregator branch from 62833ba to a3b5a6d Compare April 22, 2018 14:43

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

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

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

gazoakley mentioned this pull request Jun 3, 2018

New Resource: r/aws_config_authorization #4263

Merged

bflad approved these changes Jun 3, 2018

View reviewed changes

bflad merged commit a3b5a6d into hashicorp:master Jun 3, 2018

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

Update CHANGELOG for #4262

db643ec

hashicorp locked and limited conversation to collaborators Mar 30, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: r/aws_config_aggregator #4262

New Resource: r/aws_config_aggregator #4262

gazoakley commented Apr 19, 2018 •

edited

djk commented May 22, 2018

gazoakley commented May 22, 2018

djk commented May 22, 2018

bflad left a comment

bflad Jun 3, 2018

bflad Jun 3, 2018

bflad Jun 3, 2018

gazoakley 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

bflad Jun 3, 2018

bflad Jun 3, 2018

bflad commented Jun 5, 2018

msvechla commented Apr 8, 2019 •

edited

guipaivanz commented Apr 17, 2019

guipaivanz commented Apr 17, 2019

ghost commented Mar 30, 2020


		## Attributes Reference

		The following attributes are exported:

New Resource: r/aws_config_aggregator #4262

New Resource: r/aws_config_aggregator #4262

Conversation

gazoakley commented Apr 19, 2018 • edited

djk commented May 22, 2018

gazoakley commented May 22, 2018

djk commented May 22, 2018

bflad 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Jun 5, 2018

msvechla commented Apr 8, 2019 • edited

guipaivanz commented Apr 17, 2019

guipaivanz commented Apr 17, 2019

ghost commented Mar 30, 2020

gazoakley commented Apr 19, 2018 •

edited

msvechla commented Apr 8, 2019 •

edited