#713 Support autoscaling on EMR #2877

darrenhaken · 2018-01-05T16:51:04Z

Can someone review this?

darrenhaken · 2018-01-05T18:02:26Z

This links to #713

darrenhaken · 2018-01-05T18:17:28Z

I have added the docs now so it should be good to merge in

darrenhaken · 2018-01-06T07:48:19Z

@jen20 any idea when this might get reviewed?

bflad · 2018-01-10T00:57:39Z

Hi @darrenhaken, thanks for your work here. Due to the fast moving nature of the AWS SDK versions, we tend to prefer vendor/dependency changes in a separate PR to prevent rebase issues like the conflicts now in this PR. Can you please rebase so it doesn't include any vendor changes? The master branch has v1.12.57 right now.

darrenhaken · 2018-01-10T11:34:16Z

@bflad should be fixed now, can you review the PR?

darrenhaken · 2018-01-15T09:32:34Z

@bflad has this been reviewed yet? I'd like to get cracking on another PR.

darrenhaken · 2018-02-06T16:40:23Z

@bflad any update on this? It's been around a month now and we'd really like this feature.

darrenhaken · 2018-02-09T11:59:22Z

Does anyone have an update on this? To go a month without any feedback doesn't fill me with motivation to keep contributing to TF 👎

bflad · 2018-02-09T18:29:38Z

Hi @darrenhaken, this is my fault for dropping the ball here as you've been explicitly asking me for an update (rightfully since I asked you to change something). I'm really sorry about that. 😢 We have a lot going on in this repository and this got shuffled in with the various other issue/PR notifications.

I will give an actual review of this later today.

bflad

One minor change and rebase with the latest documentation, then LGTM. Thanks for the contribution and sorry for the delayed review.

=== RUN   TestAccAWSEMRCluster_terminationProtected
--- PASS: TestAccAWSEMRCluster_terminationProtected (387.41s)
=== RUN   TestAccAWSEMRCluster_bootstrap_ordering
--- PASS: TestAccAWSEMRCluster_bootstrap_ordering (432.28s)
=== RUN   TestAccAWSEMRCluster_security_config
--- PASS: TestAccAWSEMRCluster_security_config (442.92s)
=== RUN   TestAccAWSEMRCluster_basic
--- PASS: TestAccAWSEMRCluster_basic (525.40s)
=== RUN   TestAccAWSEMRCluster_instance_group
--- PASS: TestAccAWSEMRCluster_instance_group (532.58s)
=== RUN   TestAccAWSEMRCluster_custom_ami_id
--- PASS: TestAccAWSEMRCluster_custom_ami_id (623.71s)
=== RUN   TestAccAWSEMRCluster_s3Logging
--- PASS: TestAccAWSEMRCluster_s3Logging (634.92s)
=== RUN   TestAccAWSEMRCluster_visibleToAllUsers
--- PASS: TestAccAWSEMRCluster_visibleToAllUsers (773.45s)
=== RUN   TestAccAWSEMRCluster_root_volume_size
--- PASS: TestAccAWSEMRCluster_root_volume_size (969.56s)
=== RUN   TestAccAWSEMRCluster_tags
--- PASS: TestAccAWSEMRCluster_tags (1011.22s)

bflad · 2018-02-09T19:47:41Z

aws/resource_aws_emr_cluster.go

+						"autoscaling_policy": {
+							Type:             schema.TypeString,
+							Optional:         true,
+							DiffSuppressFunc: suppressEquivalentAwsPolicyDiffs,


Despite the naming, suppressEquivalentAwsPolicyDiffs is currently only for IAM-type policy documents. In this case we'll likely be looking for this combination of attribute handlers:

DiffSuppressFunc: suppressEquivalentJsonDiffs, ValidateFunc: validateJsonString, StateFunc: func(v interface{}) string { json, _ := structure.NormalizeJsonString(v) return json },

I'll make the change now

@bflad

- As per the review process for the PR with @bflad - https://github.com/terraform-providers/terraform-provider-aws/pull/2877/files/7307a17ae67702f28be85d052e7e922e00bbd9e5#diff-4bc1c34afa1c12afe92754aeb99c560c

darrenhaken

This should be resolved now, can you review it for me? I'll work on my next PR once we have this one through 👍

darrenhaken · 2018-02-13T08:58:19Z

aws/resource_aws_emr_cluster.go

+						"autoscaling_policy": {
+							Type:             schema.TypeString,
+							Optional:         true,
+							DiffSuppressFunc: suppressEquivalentAwsPolicyDiffs,


I'll make the change now

bflad · 2018-02-13T11:16:05Z

Almost! make test is currently failing, https://travis-ci.org/terraform-providers/terraform-provider-aws/builds/340856573:

$ make test
==> Checking that code complies with gofmt requirements...
go test ./... -timeout=30s -parallel=4
# github.com/terraform-providers/terraform-provider-aws/aws
aws/resource_aws_emr_cluster.go:18:2: imported and not used: "github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/helper/structure"
aws/resource_aws_emr_cluster.go:183:26: undefined: normalizeJsonString
# github.com/terraform-providers/terraform-provider-aws/aws
aws/resource_aws_emr_cluster.go:18:2: imported and not used: "github.com/terraform-providers/terraform-provider-aws/vendor/github.com/hashicorp/terraform/helper/structure"
aws/resource_aws_emr_cluster.go:183:26: undefined: normalizeJsonString
make: *** [test] Error 2

Some of the active contributors/maintainers here like using goimports to automatically manage imports on file save. It has integrations with most editors. 😄

darrenhaken · 2018-02-13T14:50:23Z

@bflad oops! Thanks for the tip on goimports.

It all looks to be passing now, can you review?

bflad · 2018-02-15T21:42:54Z

LGTM 🚀

Tests passed: 10
=== RUN   TestAccAWSEMRCluster_basic
--- PASS: TestAccAWSEMRCluster_basic (450.84s)
=== RUN   TestAccAWSEMRCluster_instance_group
--- PASS: TestAccAWSEMRCluster_instance_group (473.68s)
=== RUN   TestAccAWSEMRCluster_terminationProtected
--- PASS: TestAccAWSEMRCluster_terminationProtected (480.30s)
=== RUN   TestAccAWSEMRCluster_bootstrap_ordering
--- PASS: TestAccAWSEMRCluster_bootstrap_ordering (491.71s)
=== RUN   TestAccAWSEMRCluster_security_config
--- PASS: TestAccAWSEMRCluster_security_config (492.46s)
=== RUN   TestAccAWSEMRCluster_custom_ami_id
--- PASS: TestAccAWSEMRCluster_custom_ami_id (576.59s)
=== RUN   TestAccAWSEMRCluster_s3Logging
--- PASS: TestAccAWSEMRCluster_s3Logging (644.34s)
=== RUN   TestAccAWSEMRCluster_tags
--- PASS: TestAccAWSEMRCluster_tags (818.61s)
=== RUN   TestAccAWSEMRCluster_root_volume_size
--- PASS: TestAccAWSEMRCluster_root_volume_size (862.40s)
=== RUN   TestAccAWSEMRCluster_visibleToAllUsers
--- PASS: TestAccAWSEMRCluster_visibleToAllUsers (996.15s)

bflad · 2018-02-27T12:52:42Z

This has been released in version 1.10.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-07T17:53:02Z

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!

darrenhaken mentioned this pull request Jan 5, 2018

Support for AWS Auto Scaling EMR Clusters #713

Closed

darrenhaken changed the title ~~[WIP] Add autoscaling policy on EMR instance groups #713~~ [WIP] #713 Add autoscaling policy on EMR instance groups Jan 5, 2018

darrenhaken changed the title ~~[WIP] #713 Add autoscaling policy on EMR instance groups~~ #713 Add autoscaling policy on EMR instance groups Jan 5, 2018

darrenhaken changed the title ~~#713 Add autoscaling policy on EMR instance groups~~ #713 Support autoscaling EMR Jan 5, 2018

darrenhaken changed the title ~~#713 Support autoscaling EMR~~ #713 Support autoscaling on EMR Jan 5, 2018

darrenhaken mentioned this pull request Jan 5, 2018

Added support for EMR AutoScalingRole hashicorp/terraform#12823

Merged

jen20 added enhancement Requests to existing resources that expand the functionality or scope. service/emr Issues and PRs that pertain to the emr service. labels Jan 6, 2018

darrenhaken added 2 commits January 10, 2018 11:32

Add autoscaling policy on EMR instance groups hashicorp#713

f0e077d

Add EMR docs for autoscaling instace group hashicorp#713

7307a17

darrenhaken force-pushed the master branch from 62ca114 to 7307a17 Compare January 10, 2018 11:33

bflad self-requested a review February 9, 2018 18:30

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

bflad reviewed Feb 9, 2018

View reviewed changes

Change emr autoscaling to use validate json func

3d79beb

- As per the review process for the PR with @bflad - https://github.com/terraform-providers/terraform-provider-aws/pull/2877/files/7307a17ae67702f28be85d052e7e922e00bbd9e5#diff-4bc1c34afa1c12afe92754aeb99c560c

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

Merge remote-tracking branch 'upstream/master'

b10b22b

darrenhaken commented Feb 13, 2018

View reviewed changes

Fix build error

4c48218

bflad approved these changes Feb 15, 2018

View reviewed changes

bflad merged commit 04efc1d into hashicorp:master Feb 15, 2018

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

Update CHANGELOG for #2877

503e644

ghost locked and limited conversation to collaborators Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#713 Support autoscaling on EMR #2877

#713 Support autoscaling on EMR #2877

darrenhaken commented Jan 5, 2018 •

edited

Loading

darrenhaken commented Jan 5, 2018

darrenhaken commented Jan 5, 2018

darrenhaken commented Jan 6, 2018

bflad commented Jan 10, 2018

darrenhaken commented Jan 10, 2018

darrenhaken commented Jan 15, 2018

darrenhaken commented Feb 6, 2018

darrenhaken commented Feb 9, 2018 •

edited

Loading

bflad commented Feb 9, 2018

bflad left a comment •

edited

Loading

bflad Feb 9, 2018

darrenhaken Feb 13, 2018

darrenhaken left a comment

darrenhaken Feb 13, 2018

bflad commented Feb 13, 2018

darrenhaken commented Feb 13, 2018 •

edited

Loading

bflad commented Feb 15, 2018

bflad commented Feb 27, 2018

ghost commented Apr 7, 2020

#713 Support autoscaling on EMR #2877

#713 Support autoscaling on EMR #2877

Conversation

darrenhaken commented Jan 5, 2018 • edited Loading

darrenhaken commented Jan 5, 2018

darrenhaken commented Jan 5, 2018

darrenhaken commented Jan 6, 2018

bflad commented Jan 10, 2018

darrenhaken commented Jan 10, 2018

darrenhaken commented Jan 15, 2018

darrenhaken commented Feb 6, 2018

darrenhaken commented Feb 9, 2018 • edited Loading

bflad commented Feb 9, 2018

bflad left a comment • edited Loading

Choose a reason for hiding this comment

bflad Feb 9, 2018

Choose a reason for hiding this comment

darrenhaken Feb 13, 2018

Choose a reason for hiding this comment

darrenhaken left a comment

Choose a reason for hiding this comment

darrenhaken Feb 13, 2018

Choose a reason for hiding this comment

bflad commented Feb 13, 2018

darrenhaken commented Feb 13, 2018 • edited Loading

bflad commented Feb 15, 2018

bflad commented Feb 27, 2018

ghost commented Apr 7, 2020

darrenhaken commented Jan 5, 2018 •

edited

Loading

darrenhaken commented Feb 9, 2018 •

edited

Loading

bflad left a comment •

edited

Loading

darrenhaken commented Feb 13, 2018 •

edited

Loading