EMR Cluster Import Support #6498

mbfrahry · 2018-11-16T18:47:58Z

This PR finalizes adding support for Import on emr clusters #6498.
It fixes an issue where changing an optional value in a Set would force the recreation of the whole resource.
It removes all log errors in favor of returning an error message #3652.

…anges.

…undin/terraform-provider-aws into stefansundin-aws_emr_cluster-importer

… tests

bflad

Oh, EMR. 😅 Feel free to ping me directly to discuss anything!

bflad · 2018-11-26T01:23:26Z

aws/resource_aws_emr_cluster.go

@@ -202,14 +211,16 @@ func resourceAwsEMRCluster() *schema.Resource {
 							Required: false,
 						},
 						"ebs_config": {
-							Type:     schema.TypeSet,
+							Type:     schema.TypeList,


Was this change intentional? If so, we should also set MaxItems: 1 as well - otherwise we should undo this change as its not reliable to migrate a Terraform state and configuration from a multiple element TypeSet to TypeList.

It was intentional because of another issue that cropped up. I've since fixed both of these issues and we're back to TypeSet on ebs_config

bflad · 2018-11-26T01:40:27Z

aws/resource_aws_emr_cluster.go

@@ -238,6 +249,7 @@ func resourceAwsEMRCluster() *schema.Resource {
 							Optional:         true,
 							DiffSuppressFunc: suppressEquivalentJsonDiffs,
 							ValidateFunc:     validation.ValidateJsonString,
+							Computed:         true,


I'm trying not to introduce too much further scope creep into this pull request, but can you please add acceptance testing that attempts to update autoscaling_policy? I have a hunch that it will result in a permanent difference, especially with the new Set function. I think the same problem also applies to bid_price, instance_count, and especially instance_role. These attributes likely should be ForceNew: true if I'm right, but maybe I'm wrong.

I'm also not opposed to updating the resource documentation to discourage the usage of instance_group in preference of the (upcoming) aws_emr_instance_fleet and aws_emr_instance_group resources.

bflad · 2018-11-26T01:50:05Z

aws/resource_aws_emr_cluster.go


-			ebsConfig = append(ebsConfig, ebsAttrs)
+		if ig.EbsBlockDevices != nil {


range automatically can handle nil slices, so I don't believe this nil check is necessary: https://play.golang.org/p/-9ZKxKImPFH

bflad · 2018-11-26T01:54:29Z

aws/resource_aws_emr_cluster.go

+			autoscalingPolicyBytes, err := json.Marshal(ig.AutoScalingPolicy)
+			autoscalingPolicyString := string(autoscalingPolicyBytes)
+			if err != nil {
+				return nil, err


Nit: We should probably provide some context about this error here:

Suggested change

return nil, err

return nil, fmt.Errorf("error parsing EMR Cluster Instance Groups AutoScalingPolicy: %s", err)

bflad · 2018-11-26T01:57:03Z

aws/resource_aws_emr_cluster_test.go

+				ImportStateVerify: true,
+				ImportStateVerifyIgnore: []string{
+					"configurations",
+					"core_instance_count",


Should this still be ignoring the core_instance_* attributes?

bflad · 2018-11-26T01:57:38Z

aws/resource_aws_emr_cluster_test.go

+				ImportStateVerify: true,
+				ImportStateVerifyIgnore: []string{
+					"configurations",
+					"core_instance_count",


Should this still be ignoring core_instance_* attributes?

mbfrahry · 2019-01-07T03:17:37Z

@bflad, this is ready for another pass. I've swapped everything back to Sets and added hashcodes to each one. Also, I'm doing a funky thing with CustomizeDiff that is preventing changing the autoscaling policy for an instance group to recreate the whole resource.

make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSEMRCluster'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws/ -v -parallel 20 -run=TestAccAWSEMRCluster -timeout 120m
=== RUN   TestAccAWSEMRCluster_basic
=== PAUSE TestAccAWSEMRCluster_basic
=== RUN   TestAccAWSEMRCluster_additionalInfo
=== PAUSE TestAccAWSEMRCluster_additionalInfo
=== RUN   TestAccAWSEMRCluster_configurationsJson
=== PAUSE TestAccAWSEMRCluster_configurationsJson
=== RUN   TestAccAWSEMRCluster_instance_group
=== PAUSE TestAccAWSEMRCluster_instance_group
=== RUN   TestAccAWSEMRCluster_instance_group_update
=== PAUSE TestAccAWSEMRCluster_instance_group_update
=== RUN   TestAccAWSEMRCluster_instance_group_EBSVolumeType_st1
=== PAUSE TestAccAWSEMRCluster_instance_group_EBSVolumeType_st1
=== RUN   TestAccAWSEMRCluster_updateAutoScalingPolicy
=== PAUSE TestAccAWSEMRCluster_updateAutoScalingPolicy
=== RUN   TestAccAWSEMRCluster_Kerberos_ClusterDedicatedKdc
=== PAUSE TestAccAWSEMRCluster_Kerberos_ClusterDedicatedKdc
=== RUN   TestAccAWSEMRCluster_security_config
=== PAUSE TestAccAWSEMRCluster_security_config
=== RUN   TestAccAWSEMRCluster_Step_Basic
=== PAUSE TestAccAWSEMRCluster_Step_Basic
=== RUN   TestAccAWSEMRCluster_Step_Multiple
=== PAUSE TestAccAWSEMRCluster_Step_Multiple
=== RUN   TestAccAWSEMRCluster_bootstrap_ordering
=== PAUSE TestAccAWSEMRCluster_bootstrap_ordering
=== RUN   TestAccAWSEMRCluster_terminationProtected
=== PAUSE TestAccAWSEMRCluster_terminationProtected
=== RUN   TestAccAWSEMRCluster_keepJob
=== PAUSE TestAccAWSEMRCluster_keepJob
=== RUN   TestAccAWSEMRCluster_visibleToAllUsers
=== PAUSE TestAccAWSEMRCluster_visibleToAllUsers
=== RUN   TestAccAWSEMRCluster_s3Logging
=== PAUSE TestAccAWSEMRCluster_s3Logging
=== RUN   TestAccAWSEMRCluster_tags
=== PAUSE TestAccAWSEMRCluster_tags
=== RUN   TestAccAWSEMRCluster_root_volume_size
=== PAUSE TestAccAWSEMRCluster_root_volume_size
=== RUN   TestAccAWSEMRCluster_custom_ami_id
=== PAUSE TestAccAWSEMRCluster_custom_ami_id
=== CONT  TestAccAWSEMRCluster_basic
=== CONT  TestAccAWSEMRCluster_Step_Multiple
=== CONT  TestAccAWSEMRCluster_tags
=== CONT  TestAccAWSEMRCluster_visibleToAllUsers
=== CONT  TestAccAWSEMRCluster_s3Logging
=== CONT  TestAccAWSEMRCluster_root_volume_size
=== CONT  TestAccAWSEMRCluster_custom_ami_id
=== CONT  TestAccAWSEMRCluster_instance_group
=== CONT  TestAccAWSEMRCluster_instance_group_EBSVolumeType_st1
=== CONT  TestAccAWSEMRCluster_Step_Basic
=== CONT  TestAccAWSEMRCluster_security_config
=== CONT  TestAccAWSEMRCluster_Kerberos_ClusterDedicatedKdc
=== CONT  TestAccAWSEMRCluster_instance_group_update
=== CONT  TestAccAWSEMRCluster_terminationProtected
=== CONT  TestAccAWSEMRCluster_keepJob
=== CONT  TestAccAWSEMRCluster_configurationsJson
=== CONT  TestAccAWSEMRCluster_bootstrap_ordering
=== CONT  TestAccAWSEMRCluster_updateAutoScalingPolicy
=== CONT  TestAccAWSEMRCluster_additionalInfo
--- PASS: TestAccAWSEMRCluster_instance_group_EBSVolumeType_st1 (503.34s)
--- PASS: TestAccAWSEMRCluster_additionalInfo (543.40s)
--- PASS: TestAccAWSEMRCluster_custom_ami_id (569.04s)
--- PASS: TestAccAWSEMRCluster_security_config (591.30s)
--- PASS: TestAccAWSEMRCluster_Step_Multiple (609.49s)
--- PASS: TestAccAWSEMRCluster_terminationProtected (609.60s)
--- PASS: TestAccAWSEMRCluster_bootstrap_ordering (617.58s)
--- PASS: TestAccAWSEMRCluster_configurationsJson (638.84s)
--- PASS: TestAccAWSEMRCluster_updateAutoScalingPolicy (642.36s)
--- PASS: TestAccAWSEMRCluster_basic (643.61s)
--- PASS: TestAccAWSEMRCluster_instance_group_update (645.28s)
--- PASS: TestAccAWSEMRCluster_Kerberos_ClusterDedicatedKdc (648.19s)
--- PASS: TestAccAWSEMRCluster_instance_group (666.50s)
--- PASS: TestAccAWSEMRCluster_Step_Basic (678.44s)
--- PASS: TestAccAWSEMRCluster_keepJob (782.12s)
--- PASS: TestAccAWSEMRCluster_s3Logging (861.22s)
--- PASS: TestAccAWSEMRCluster_visibleToAllUsers (926.71s)
--- PASS: TestAccAWSEMRCluster_root_volume_size (1155.21s)
--- PASS: TestAccAWSEMRCluster_tags (1221.14s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	1221.773s

bflad

Let's get this in. Thanks so much @mbfrahry and @tracypholmes for your dedication here! 🚀 🎉

--- PASS: TestAccAWSEMRCluster_Kerberos_ClusterDedicatedKdc (468.11s)
--- PASS: TestAccAWSEMRCluster_security_config (502.72s)
--- PASS: TestAccAWSEMRCluster_instance_group_EBSVolumeType_st1 (515.43s)
--- PASS: TestAccAWSEMRCluster_bootstrap_ordering (518.55s)
--- PASS: TestAccAWSEMRCluster_instance_group (523.49s)
--- PASS: TestAccAWSEMRCluster_keepJob (533.63s)
--- PASS: TestAccAWSEMRCluster_custom_ami_id (537.09s)
--- PASS: TestAccAWSEMRCluster_instance_group_update (551.88s)
--- PASS: TestAccAWSEMRCluster_Step_Basic (576.80s)
--- PASS: TestAccAWSEMRCluster_basic (577.48s)
--- PASS: TestAccAWSEMRCluster_configurationsJson (585.18s)
--- PASS: TestAccAWSEMRCluster_Step_Multiple (594.08s)
--- PASS: TestAccAWSEMRCluster_terminationProtected (658.88s)
--- PASS: TestAccAWSEMRCluster_s3Logging (688.12s)
--- PASS: TestAccAWSEMRCluster_updateAutoScalingPolicy (690.46s)
--- PASS: TestAccAWSEMRCluster_additionalInfo (823.56s)
--- PASS: TestAccAWSEMRCluster_visibleToAllUsers (902.38s)
--- PASS: TestAccAWSEMRCluster_root_volume_size (919.39s)
--- PASS: TestAccAWSEMRCluster_tags (989.43s)

bflad · 2019-01-16T04:19:28Z

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

jhart-r7 · 2019-01-24T19:09:08Z

I believe this may have introduced a bug when using the optional instance_group which causes the EMR cluster to change upon every apply. See #7278.

ghost · 2020-04-01T17:22:50Z

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!

stefansundin and others added 11 commits August 20, 2018 16:08

Add Importer to aws_emr_cluster.

4fc9827

Add import help to aws_emr_cluster documentation. Plus other minor ch…

8c54895

…anges.

Fix typos in aws_emr_security_configuration documentation.

1637bb3

Add acceptance test for import.

6cbb601

Make TestAccAWSEMRCluster_basic pass.

2c1c04e

Remove duplicate code.

015ae93

Merge branch 'aws_emr_cluster-importer' of https://github.com/stefans…

d5760ad

…undin/terraform-provider-aws into stefansundin-aws_emr_cluster-importer

tests/resource/aws_emr_cluster: Add Import TestStep to all acceptance…

ff3c579

… tests

Merge branch 'master' into aws_emr_cluster-importer

2d2bb36

Adding import support

b359cd4

Updating tests

9e50594

tracypholmes and others added 2 commits November 16, 2018 14:11

Fixing tests

063db7f

Finalizing tests

4387c3a

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 Nov 20, 2018

Removing comments

c833636

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

mbfrahry requested review from tracypholmes and bflad November 20, 2018 18:06

bflad reviewed Nov 26, 2018

View reviewed changes

Adding migration steps to EMR Cluster

2fa32bd

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 Dec 10, 2018

mbfrahry added 2 commits December 16, 2018 23:09

Continuing work on emr clusters

adffd0d

adding customize diff

a381f6b

mbfrahry added 2 commits January 2, 2019 14:04

Merging with master

d3c6e20

Fixing tests

c2c9d2a

ghost added size/XXL Managed by automation to categorize the size of a PR. and removed size/XL Managed by automation to categorize the size of a PR. labels Jan 3, 2019

mbfrahry added 3 commits January 6, 2019 08:33

Finalizing tests

c352a2d

Additional tests and website additions

566e5b1

Removing additional files and extraneous comments

c0af366

Simplifying loops

609bb1e

mbfrahry requested a review from a team January 10, 2019 23:51

Adding 0.12 supports and fixing docs

8e76a7f

bflad added this to the v1.56.0 milestone Jan 11, 2019

bflad approved these changes Jan 11, 2019

View reviewed changes

bflad merged commit 2f99563 into master Jan 11, 2019

bflad deleted the stefansundin-aws_emr_cluster-importer branch January 11, 2019 16:14

bflad mentioned this pull request Jan 11, 2019

Investigate aws_emr_cluster d.Set() errors only logging instead of returning errors #3652

Closed

bflad added a commit that referenced this pull request Jan 11, 2019

Update CHANGELOG for #4488 and #6498

6e8d594

mbfrahry changed the title ~~[WIP] EMR Cluster Import Support~~ EMR Cluster Import Support Jan 11, 2019

jhart-r7 mentioned this pull request Jan 23, 2019

Unexpected changes with aws_emr_cluster when using instance_group list #7278

Closed

ghost locked and limited conversation to collaborators Apr 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EMR Cluster Import Support #6498

EMR Cluster Import Support #6498

mbfrahry commented Nov 16, 2018 •

edited

Loading

bflad left a comment

bflad Nov 26, 2018

mbfrahry Dec 5, 2018

bflad Nov 26, 2018

bflad Nov 26, 2018

bflad Nov 26, 2018

bflad Nov 26, 2018

bflad Nov 26, 2018

mbfrahry commented Jan 7, 2019

bflad left a comment

bflad commented Jan 16, 2019

jhart-r7 commented Jan 24, 2019

ghost commented Apr 1, 2020


		ebsConfig = append(ebsConfig, ebsAttrs)
		if ig.EbsBlockDevices != nil {

	return nil, err
	return nil, fmt.Errorf("error parsing EMR Cluster Instance Groups AutoScalingPolicy: %s", err)

EMR Cluster Import Support #6498

EMR Cluster Import Support #6498

Conversation

mbfrahry commented Nov 16, 2018 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

bflad Nov 26, 2018

Choose a reason for hiding this comment

mbfrahry Dec 5, 2018

Choose a reason for hiding this comment

bflad Nov 26, 2018

Choose a reason for hiding this comment

bflad Nov 26, 2018

Choose a reason for hiding this comment

bflad Nov 26, 2018

Choose a reason for hiding this comment

bflad Nov 26, 2018

Choose a reason for hiding this comment

bflad Nov 26, 2018

Choose a reason for hiding this comment

mbfrahry commented Jan 7, 2019

bflad left a comment

Choose a reason for hiding this comment

bflad commented Jan 16, 2019

jhart-r7 commented Jan 24, 2019

ghost commented Apr 1, 2020

mbfrahry commented Nov 16, 2018 •

edited

Loading