Add attribute configurations_json in aws_emr_cluster #5191

saravanan30erd · 2018-07-14T10:33:55Z

Output from acceptance testing:

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

saravanan30erd · 2018-07-14T11:11:09Z

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

bflad

Thanks @saravanan30erd! Please see the below and let us know if you have any questions.

bflad · 2018-07-17T13:17:07Z

aws/resource_aws_emr_cluster.go

@@ -666,6 +685,10 @@ func resourceAwsEMRClusterRead(d *schema.ResourceData, meta interface{}) error {
 		log.Printf("[ERR] Error setting EMR configurations for cluster (%s): %s", d.Id(), err)
 	}

+	if err := d.Set("configurations_json", cluster.Configurations); err != nil {
+		log.Printf("[ERR] Error setting EMR configurations_json for cluster (%s): %s", d.Id(), err)


We should prefer to return an error message instead of logging when d.Set() fails. We will fix the others as part of #3652

@bflad when I returned the error message instead of logging, I came to know there is an error while setting the configurations_json. So I tried to decode the API cluster. Configurations output as json string using json.Marshal but I got an below error,

configurations_json: "[{\"Classification\":\"hadoop-env\",\"Configurations\":[{\"Classification\":\"export\",\"Configurations\":null,\"Properties\":{\"JAVA_HOME\":\"/usr/lib/jvm/java-1.8.0\"}}],\"Properties\":{}},{\"Classification\":\"spark-env\",\"Configurations\":[{\"Classification\":\"export\",\"Configurations\":null,\"Properties\":{\"JAVA_HOME\":\"/usr/lib/jvm/java-1.8.0\"}}],\"Properties\":{}}]" => "[{\"Classification\":\"hadoop-env\",\"Configurations\":[{\"Classification\":\"export\",\"Configurations\":[],\"Properties\":{\"JAVA_HOME\":\"/usr/lib/jvm/java-1.8.0\"}}],\"Properties\":{}},{\"Classification\":\"spark-env\",\"Configurations\":[{\"Classification\":\"export\",\"Configurations\":[],\"Properties\":{\"JAVA_HOME\":\"/usr/lib/jvm/java-1.8.0\"}}],\"Properties\":{}}]" (forces new resource)

since API returning nil (for []*emr.Configuration), json.Marshal converts it into null and causes the above issue. Looks like we can rewrite API output from nil to []*emr.Configuration{} to fix this, but emr.Configuration type have field Configurations which value is again []*emr.Configuration and it goes on. May be recursive function helps this case.
But I tried jsonutil.BuildJSON (https://godoc.org/github.com/aws/aws-sdk-go/private/protocol/json/jsonutil) to decode json, and it completely skips the field(Configurations), since it have nil value. so I think we can use this and we can update the document like, if Configurations is empty please completely skip the field
Configurations instead of providing empty list "Configurations": [] in input json same as first example (https://docs.aws.amazon.com/emr/latest/ReleaseGuide/emr-configure-apps.html).
Any thoughts?

bflad · 2018-07-17T13:18:24Z

aws/resource_aws_emr_cluster.go

+	configsOut := []*emr.Configuration{}
+	err := json.Unmarshal([]byte(input), &configsOut)
+	if err != nil {
+		log.Printf("[ERROR] parsing JSON: %s", err)


It seems like we should be returning this error to the operator instead of just logging it.

bflad · 2018-07-17T13:19:55Z

aws/resource_aws_emr_cluster_test.go

+  core_instance_type   = "c4.large"
+  core_instance_count  = 1
+
+  tags {


Minor nitpick: Can all the extraneous optional arguments please be removed? This test configuration only needs to worry about configurations_json and the other arguments can otherwise introduce false positives.

bflad · 2018-07-17T13:22:50Z

aws/resource_aws_emr_cluster.go

@@ -350,6 +350,17 @@ func resourceAwsEMRCluster() *schema.Resource {
 				ForceNew: true,
 				Optional: true,
 			},
+			"configurations_json": {


We should prefer to also deprecate the configurations attribute in this process to prevent further usage after the next major release. 👍 The two attributes should also conflict with each other as well.

bflad

This is currently failing acceptance testing on every other test that defines configurations, which will be a requirement for backwards compatibility. e.g.

=== RUN   TestAccAWSEMRCluster_basic
--- FAIL: TestAccAWSEMRCluster_basic (463.33s)
	testing.go:518: Step 0 error: After applying this step, the plan was not empty:
		
		DIFF:
		
		DESTROY/CREATE: aws_emr_cluster.tf-test-cluster
...
		  configurations:                                     "test-fixtures/emr_configurations.json" => "test-fixtures/emr_configurations.json"
		  configurations_json:                                "[{\"Classification\":\"hadoop-env\",\"Configurations\":[{\"Classification\":\"export\",\"Properties\":{\"JAVA_HOME\":\"/usr/lib/jvm/java-1.8.0\"}}],\"Properties\":{}},{\"Classification\":\"spark-env\",\"Configurations\":[{\"Classification\":\"export\",\"Properties\":{\"JAVA_HOME\":\"/usr/lib/jvm/java-1.8.0\"}}],\"Properties\":{}}]" => "" (forces new resource)

We should only d.Set("configurations_json") if d.GetOk("configurations_json") passes or configurations is empty. Something like this should help:

if _, ok := d.GetOk("configurations_json"); ok {
	configOut, err := flattenConfigurationJson(cluster.Configurations)
 	if err != nil {
 		return fmt.Errorf("Error reading EMR cluster configurations: %s", err)
 	}
 	if err := d.Set("configurations_json", configOut); err != nil {
 		return fmt.Errorf("Error setting EMR configurations_json for cluster (%s): %s", d.Id(), err)
 	}
}

saravanan30erd · 2018-08-01T15:19:37Z

@bflad thanks, done the changes.

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

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

bflad

LGTM, thanks @saravanan30erd! 🚀 We'll followup with a deprecation plan for the existing configurations attribute shortly.

15 tests passed (all tests)
=== RUN   TestAccAWSEMRCluster_basic
--- PASS: TestAccAWSEMRCluster_basic (385.79s)
=== RUN   TestAccAWSEMRCluster_security_config
--- PASS: TestAccAWSEMRCluster_security_config (424.55s)
=== RUN   TestAccAWSEMRCluster_Kerberos_ClusterDedicatedKdc
--- PASS: TestAccAWSEMRCluster_Kerberos_ClusterDedicatedKdc (461.56s)
=== RUN   TestAccAWSEMRCluster_Step_Basic
--- PASS: TestAccAWSEMRCluster_Step_Basic (464.90s)
=== RUN   TestAccAWSEMRCluster_bootstrap_ordering
--- PASS: TestAccAWSEMRCluster_bootstrap_ordering (474.40s)
=== RUN   TestAccAWSEMRCluster_instance_group
--- PASS: TestAccAWSEMRCluster_instance_group (484.81s)
=== RUN   TestAccAWSEMRCluster_configurationsJson
--- PASS: TestAccAWSEMRCluster_configurationsJson (487.48s)
=== RUN   TestAccAWSEMRCluster_terminationProtected
--- PASS: TestAccAWSEMRCluster_terminationProtected (495.84s)
=== RUN   TestAccAWSEMRCluster_Step_Multiple
--- PASS: TestAccAWSEMRCluster_Step_Multiple (505.27s)
=== RUN   TestAccAWSEMRCluster_custom_ami_id
--- PASS: TestAccAWSEMRCluster_custom_ami_id (545.79s)
=== RUN   TestAccAWSEMRCluster_additionalInfo
--- PASS: TestAccAWSEMRCluster_additionalInfo (560.31s)
=== RUN   TestAccAWSEMRCluster_s3Logging
--- PASS: TestAccAWSEMRCluster_s3Logging (601.98s)
=== RUN   TestAccAWSEMRCluster_root_volume_size
--- PASS: TestAccAWSEMRCluster_root_volume_size (764.59s)
=== RUN   TestAccAWSEMRCluster_tags
--- PASS: TestAccAWSEMRCluster_tags (764.74s)
=== RUN   TestAccAWSEMRCluster_visibleToAllUsers
--- PASS: TestAccAWSEMRCluster_visibleToAllUsers (934.24s)

bflad · 2018-08-02T13:43:34Z

This has been released in version 1.30.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-04T17:23:34Z

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!

saravanan30erd added 3 commits July 14, 2018 12:21

add attribute configurations_json

2689979

add acceptance test for configurations_json

8d5f859

add documentation for configurations_json

6334a25

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

update acceptance test for configurations_json

c645f4c

bflad added bug Addresses a defect in current functionality. service/emr Issues and PRs that pertain to the emr service. labels Jul 16, 2018

bflad reviewed Jul 17, 2018

View reviewed changes

fix the error setting configurations_json

67a7783

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

saravanan30erd added 2 commits July 22, 2018 22:11

make attributes conflicts with each other

83425c0

fix test case for configurations_json

904b1e8

bflad reviewed Aug 1, 2018

View reviewed changes

update documentation for configurations_json

7febbf0

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

address PR hashicorp#5191 feedback

307dc3e

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

bflad added this to the v1.30.0 milestone Aug 1, 2018

bflad approved these changes Aug 1, 2018

View reviewed changes

bflad merged commit 6f30d2c into hashicorp:master Aug 1, 2018

bflad added a commit that referenced this pull request Aug 1, 2018

Update CHANGELOG for #5191

b642b6f

ghost locked and limited conversation to collaborators Apr 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add attribute configurations_json in aws_emr_cluster #5191

Add attribute configurations_json in aws_emr_cluster #5191

saravanan30erd commented Jul 14, 2018

saravanan30erd commented Jul 14, 2018

bflad left a comment

bflad Jul 17, 2018

saravanan30erd Jul 17, 2018

saravanan30erd Jul 22, 2018 •

edited

Loading

bflad Jul 17, 2018

bflad Jul 17, 2018

bflad Jul 17, 2018

bflad left a comment

saravanan30erd commented Aug 1, 2018

bflad left a comment

bflad commented Aug 2, 2018

ghost commented Apr 4, 2020

Add attribute configurations_json in aws_emr_cluster #5191

Add attribute configurations_json in aws_emr_cluster #5191

Conversation

saravanan30erd commented Jul 14, 2018

saravanan30erd commented Jul 14, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad Jul 17, 2018

Choose a reason for hiding this comment

saravanan30erd Jul 17, 2018

Choose a reason for hiding this comment

saravanan30erd Jul 22, 2018 • edited Loading

Choose a reason for hiding this comment

bflad Jul 17, 2018

Choose a reason for hiding this comment

bflad Jul 17, 2018

Choose a reason for hiding this comment

bflad Jul 17, 2018

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

saravanan30erd commented Aug 1, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented Aug 2, 2018

ghost commented Apr 4, 2020

saravanan30erd Jul 22, 2018 •

edited

Loading