AWS Glue worker_type feature #9115

Zambonilli · 2019-06-24T21:54:02Z

I have little experience in Google Go and would like an experienced set of eyes here. Especially around the logic for worker_type & number_of_workers being required/optional in opposite of max_allocation or allocated_capacity fields.

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #8257

Release note for CHANGELOG:

Supports AWS Glue optional worker type by adding the `worker_type` and `number_of_workers` parameters. `worker_type` parameter can be set to `Standard`, `G.1X` or `G.2x`.

Output from acceptance testing:

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

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSGlueJob'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSGlueJob -timeout 120m
=== RUN   TestAccAWSGlueJob_Basic
=== PAUSE TestAccAWSGlueJob_Basic
=== RUN   TestAccAWSGlueJob_AllocatedCapacity
=== PAUSE TestAccAWSGlueJob_AllocatedCapacity
=== RUN   TestAccAWSGlueJob_Command
=== PAUSE TestAccAWSGlueJob_Command
=== RUN   TestAccAWSGlueJob_DefaultArguments
=== PAUSE TestAccAWSGlueJob_DefaultArguments
=== RUN   TestAccAWSGlueJob_Description
=== PAUSE TestAccAWSGlueJob_Description
=== RUN   TestAccAWSGlueJob_ExecutionProperty
=== PAUSE TestAccAWSGlueJob_ExecutionProperty
=== RUN   TestAccAWSGlueJob_MaxRetries
=== PAUSE TestAccAWSGlueJob_MaxRetries
=== RUN   TestAccAWSGlueJob_Timeout
=== PAUSE TestAccAWSGlueJob_Timeout
=== RUN   TestAccAWSGlueJob_SecurityConfiguration
=== PAUSE TestAccAWSGlueJob_SecurityConfiguration
=== RUN   TestAccAWSGlueJob_WorkerType
=== PAUSE TestAccAWSGlueJob_WorkerType
=== RUN   TestAccAWSGlueJob_PythonShell
=== PAUSE TestAccAWSGlueJob_PythonShell
=== RUN   TestAccAWSGlueJob_MaxCapacity
=== PAUSE TestAccAWSGlueJob_MaxCapacity
=== CONT  TestAccAWSGlueJob_Basic
=== CONT  TestAccAWSGlueJob_WorkerType
=== CONT  TestAccAWSGlueJob_PythonShell
=== CONT  TestAccAWSGlueJob_ExecutionProperty
=== CONT  TestAccAWSGlueJob_SecurityConfiguration
=== CONT  TestAccAWSGlueJob_Timeout
=== CONT  TestAccAWSGlueJob_MaxCapacity
=== CONT  TestAccAWSGlueJob_MaxRetries
=== CONT  TestAccAWSGlueJob_DefaultArguments
=== CONT  TestAccAWSGlueJob_Description
=== CONT  TestAccAWSGlueJob_Command
=== CONT  TestAccAWSGlueJob_AllocatedCapacity
--- PASS: TestAccAWSGlueJob_Basic (30.01s)
--- PASS: TestAccAWSGlueJob_ExecutionProperty (35.53s)
--- PASS: TestAccAWSGlueJob_MaxRetries (41.82s)
--- PASS: TestAccAWSGlueJob_PythonShell (41.87s)
--- PASS: TestAccAWSGlueJob_MaxCapacity (52.79s)
--- PASS: TestAccAWSGlueJob_Timeout (63.17s)
--- PASS: TestAccAWSGlueJob_DefaultArguments (69.60s)
--- PASS: TestAccAWSGlueJob_Command (77.57s)
--- PASS: TestAccAWSGlueJob_WorkerType (83.86s)
--- PASS: TestAccAWSGlueJob_Description (85.55s)
--- PASS: TestAccAWSGlueJob_SecurityConfiguration (86.51s)
--- PASS: TestAccAWSGlueJob_AllocatedCapacity (133.35s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	133.406s
...

avicennax · 2019-08-21T23:37:04Z

@Zambonilli, do you know what the hold up is here on getting this merged in?

monnetchr · 2019-08-22T12:37:30Z

@Zambonilli maybe remove [WIP] from issue title?

Zambonilli · 2019-08-22T13:10:01Z

There's 325 open PRs currently for this project so I could only imagine how hard it is to manage that many PRs. Let's cut the team some slack here. I have WIP in the title because I'd like a 2nd set of eyes on this with more experience in Google Go. Can you be that 2nd set of eyes @monnetchr or @avicennax?

avicennax · 2019-08-22T19:38:45Z

Sure thing, but mind, I'm not terribly experienced in Go either.
Not pointing fingers here btw haha. I just want to understand why after two months, your seemingly correct and complete work hasn't had so much as a follow up note. I think @monnetchr is on to something, lets remove [WIP] from the title and ping the gatekeeper(s).

Zambonilli · 2019-08-23T14:55:07Z

Updated title to remove [WIP] tag.

stephencsmall · 2019-08-29T17:11:03Z

Anything need to be done to get these checks to pass? Looks like one was cancelled.

aeschright · 2019-09-03T18:11:21Z

It's ok to ignore the pr-filter-sync task. As noted above, we're catching up on a pretty hefty PR backlog so I can give this a quick glance but it may take a bit longer to give it a full review. Thanks for your patience!

website/docs/r/glue_job.html.markdown

Co-Authored-By: Audrey Eschright <[email protected]>

Zambonilli · 2019-09-04T16:44:39Z

updated the documentation per code review via @aeschright

Zambonilli · 2019-09-25T14:48:59Z

This PR should be gtg.

amitsehgal · 2019-10-04T20:02:15Z

any plans of merging this PR ?

Zambonilli · 2019-10-21T15:05:49Z

ping on this getting merged

kabeersvohra · 2019-11-20T11:41:35Z

Can this be merged? @aeschright

Zambonilli · 2019-11-20T14:33:18Z

yes

bflad

Looks good, @Zambonilli, thank you for submitting this. 🚀 Will address the nits on merge.

Output from acceptance testing:

--- PASS: TestAccAWSGlueJob_MaxCapacity (39.16s)
--- PASS: TestAccAWSGlueJob_Basic (44.86s)
--- PASS: TestAccAWSGlueJob_GlueVersion (53.40s)
--- PASS: TestAccAWSGlueJob_Command (59.63s)
--- PASS: TestAccAWSGlueJob_ExecutionProperty (69.02s)
--- PASS: TestAccAWSGlueJob_WorkerType (77.59s)
--- PASS: TestAccAWSGlueJob_AllocatedCapacity (82.80s)
--- PASS: TestAccAWSGlueJob_Timeout (86.08s)
--- PASS: TestAccAWSGlueJob_Description (92.14s)
--- PASS: TestAccAWSGlueJob_MaxRetries (92.75s)
--- PASS: TestAccAWSGlueJob_DefaultArguments (126.01s)
--- PASS: TestAccAWSGlueJob_SecurityConfiguration (126.53s)
--- PASS: TestAccAWSGlueJob_PythonShell (142.48s)

bflad · 2019-11-20T18:07:06Z

aws/resource_aws_glue_job.go

+				Type:          schema.TypeString,
+				Optional:      true,
+				ConflictsWith: []string{"allocated_capacity", "max_capacity", "allocated_capacity"},
+				ValidateFunc:  validation.StringMatch(regexp.MustCompile(`^Standard|G.1X|G.2X`), "Must be one of Standard, G.1X or G.2X. See https://docs.aws.amazon.com/glue/latest/dg/aws-glue-api-jobs-job.html"),


Nit: We typically prefer to use the validation.StringInSlice() functionality coupled with available AWS Go SDK constants for this type of validation, to limit the usage of regular expressions while also setting us up for future auto-generation opportunities, e.g.

Suggested change

ValidateFunc: validation.StringMatch(regexp.MustCompile(`^Standard|G.1X|G.2X`), "Must be one of Standard, G.1X or G.2X. See https://docs.aws.amazon.com/glue/latest/dg/aws-glue-api-jobs-job.html"),

ValidateFunc: validation.StringInSlice([]string{

glue.WorkerTypeG1x,

glue.WorkerTypeG2x,

glue.WorkerTypeStandard,

}, false),

This is not formatted through make fmt. There is two spaces vetween ValidateFunc: and validation. It's currently making all PR tests fail.

Here's the diff after pulling the latest master and running make fmt:

diff --git a/aws/resource_aws_glue_job.go b/aws/resource_aws_glue_job.go index 19468747d..4e2aa2945 100644 --- a/aws/resource_aws_glue_job.go +++ b/aws/resource_aws_glue_job.go @@ -122,7 +122,7 @@ func resourceAwsGlueJob() *schema.Resource { Type: schema.TypeString, Optional: true, ConflictsWith: []string{"allocated_capacity", "max_capacity"}, - ValidateFunc: validation.StringInSlice([]string{ + ValidateFunc: validation.StringInSlice([]string{ glue.WorkerTypeG1x, glue.WorkerTypeG2x, glue.WorkerTypeStandard,

I think I ran go fmt and not make fmt from the root per the CONTRIBUTING.md's Enhancement/Bugfix to a Resource -> Well-formed Code section. I can run make fmt and push to the PR.

No problem, fixed by 4e449b0, master is now passing

thank you, sorry for an inconvenience!

bflad · 2019-11-20T18:10:41Z

aws/resource_aws_glue_job.go

+			"worker_type": {
+				Type:          schema.TypeString,
+				Optional:      true,
+				ConflictsWith: []string{"allocated_capacity", "max_capacity", "allocated_capacity"},


Nit: allocated_capacity is declared twice

bflad · 2019-11-20T18:10:48Z

aws/resource_aws_glue_job.go

+			"number_of_workers": {
+				Type:          schema.TypeInt,
+				Optional:      true,
+				ConflictsWith: []string{"allocated_capacity", "max_capacity", "allocated_capacity"},


Nit: allocated_capacity is declared twice

Reference: #9115 (review) Output from acceptance testing: ``` --- PASS: TestAccAWSGlueJob_Basic (20.19s) --- PASS: TestAccAWSGlueJob_Timeout (41.58s) --- PASS: TestAccAWSGlueJob_Command (51.15s) --- PASS: TestAccAWSGlueJob_PythonShell (52.94s) --- PASS: TestAccAWSGlueJob_MaxRetries (57.00s) --- PASS: TestAccAWSGlueJob_MaxCapacity (58.24s) --- PASS: TestAccAWSGlueJob_AllocatedCapacity (67.58s) --- PASS: TestAccAWSGlueJob_GlueVersion (77.25s) --- PASS: TestAccAWSGlueJob_DefaultArguments (78.53s) --- PASS: TestAccAWSGlueJob_Description (82.81s) --- PASS: TestAccAWSGlueJob_ExecutionProperty (90.17s) --- PASS: TestAccAWSGlueJob_WorkerType (91.13s) --- PASS: TestAccAWSGlueJob_SecurityConfiguration (105.39s) ```

ghost · 2019-11-21T15:03:37Z

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2020-03-29T17:18:58Z

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!

Mike Lohmeier added 3 commits June 24, 2019 15:10

issue 8257 add worker_type param to AWS Glue jobs

e0d271b

Merge remote-tracking branch 'upstream/master'

dab64a0

issue 8257 added logic for number_of_workers

6ec6da0

Zambonilli marked this pull request as ready for review July 18, 2019 19:09

Zambonilli requested a review from a team July 18, 2019 19:09

Zambonilli changed the title ~~[WIP] AWS Glue worker_type feature~~ AWS Glue worker_type feature Aug 23, 2019

aeschright reviewed Sep 3, 2019

View reviewed changes

website/docs/r/glue_job.html.markdown Outdated Show resolved Hide resolved

Update website/docs/r/glue_job.html.markdown

dc59a64

Co-Authored-By: Audrey Eschright <[email protected]>

Merge branch 'master' into master

6246b77

bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Nov 20, 2019

bflad approved these changes Nov 20, 2019

View reviewed changes

bflad merged commit a074641 into hashicorp:master Nov 20, 2019

bflad added this to the v2.39.0 milestone Nov 20, 2019

bflad added a commit that referenced this pull request Nov 20, 2019

Update CHANGELOG for #9115

4dd7a00

jValdron mentioned this pull request Nov 20, 2019

resource/aws_codebuild_project: Implements git_submodules_config block #10952

Merged

ghost locked and limited conversation to collaborators Mar 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS Glue worker_type feature #9115

AWS Glue worker_type feature #9115

Zambonilli commented Jun 24, 2019 •

edited

Loading

avicennax commented Aug 21, 2019

monnetchr commented Aug 22, 2019

Zambonilli commented Aug 22, 2019

avicennax commented Aug 22, 2019

Zambonilli commented Aug 23, 2019

stephencsmall commented Aug 29, 2019

aeschright commented Sep 3, 2019

Zambonilli commented Sep 4, 2019

Zambonilli commented Sep 25, 2019

amitsehgal commented Oct 4, 2019

Zambonilli commented Oct 21, 2019

kabeersvohra commented Nov 20, 2019 •

edited

Loading

Zambonilli commented Nov 20, 2019

bflad left a comment

bflad Nov 20, 2019

jValdron Nov 20, 2019 •

edited

Loading

Zambonilli Nov 20, 2019

jValdron Nov 20, 2019 •

edited

Loading

Zambonilli Nov 21, 2019

bflad Nov 20, 2019

bflad Nov 20, 2019

ghost commented Nov 21, 2019

ghost commented Mar 29, 2020

-				ValidateFunc:  validation.StringMatch(regexp.MustCompile(`^Standard|G.1X|G.2X`), "Must be one of Standard, G.1X or G.2X. See https://docs.aws.amazon.com/glue/latest/dg/aws-glue-api-jobs-job.html"),
+				ValidateFunc:  validation.StringInSlice([]string{
+					glue.WorkerTypeG1x,
+					glue.WorkerTypeG2x,
+					glue.WorkerTypeStandard,
+				}, false),

AWS Glue worker_type feature #9115

AWS Glue worker_type feature #9115

Conversation

Zambonilli commented Jun 24, 2019 • edited Loading

Community Note

avicennax commented Aug 21, 2019

monnetchr commented Aug 22, 2019

Zambonilli commented Aug 22, 2019

avicennax commented Aug 22, 2019

Zambonilli commented Aug 23, 2019

stephencsmall commented Aug 29, 2019

aeschright commented Sep 3, 2019

Zambonilli commented Sep 4, 2019

Zambonilli commented Sep 25, 2019

amitsehgal commented Oct 4, 2019

Zambonilli commented Oct 21, 2019

kabeersvohra commented Nov 20, 2019 • edited Loading

Zambonilli commented Nov 20, 2019

bflad left a comment

Choose a reason for hiding this comment

bflad Nov 20, 2019

Choose a reason for hiding this comment

jValdron Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

Zambonilli Nov 20, 2019

Choose a reason for hiding this comment

jValdron Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

Zambonilli Nov 21, 2019

Choose a reason for hiding this comment

bflad Nov 20, 2019

Choose a reason for hiding this comment

bflad Nov 20, 2019

Choose a reason for hiding this comment

ghost commented Nov 21, 2019

ghost commented Mar 29, 2020

Zambonilli commented Jun 24, 2019 •

edited

Loading

kabeersvohra commented Nov 20, 2019 •

edited

Loading

jValdron Nov 20, 2019 •

edited

Loading

jValdron Nov 20, 2019 •

edited

Loading