New Resource: aws_glue_trigger #4464

bflad · 2018-05-06T19:30:33Z

make testacc TEST=./aws TESTARGS='-run=TestAccAWSGlueTrigger'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSGlueTrigger -timeout 120m
=== RUN   TestAccAWSGlueTrigger_Basic
--- PASS: TestAccAWSGlueTrigger_Basic (9.86s)
=== RUN   TestAccAWSGlueTrigger_Description
--- PASS: TestAccAWSGlueTrigger_Description (11.44s)
=== RUN   TestAccAWSGlueTrigger_Enabled
--- PASS: TestAccAWSGlueTrigger_Enabled (51.44s)
=== RUN   TestAccAWSGlueTrigger_Predicate
--- PASS: TestAccAWSGlueTrigger_Predicate (47.30s)
=== RUN   TestAccAWSGlueTrigger_Schedule
--- PASS: TestAccAWSGlueTrigger_Schedule (48.42s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	168.495s

tombuildsstuff

a bunch of mostly minor comments - but this otherwise LGTM 👍

tombuildsstuff · 2018-05-22T15:30:03Z

aws/resource_aws_glue_trigger.go

+			"actions": {
+				Type:     schema.TypeList,
+				Required: true,
+				MinItems: 1,


this is implied, since it's required

MinItems: 1 is not implied by Required: true on schema.TypeList

Given the below configuration and removing MinItems: 1 in the schema, it allows the 0 element list to pass through and error in the API:

resource "aws_glue_trigger" "test" { name = "%s" type = "ON_DEMAND" actions = [] }

testing.go:518: Step 0 error: Error applying: 1 error(s) occurred: * aws_glue_trigger.test: 1 error(s) occurred: * aws_glue_trigger.test: error creating Glue Trigger (tf-acc-test-123ih): InvalidInputException: Actions cannot be null or empty status code: 400, request id: f6ad3e4b-5de6-11e8-9ff4-73cc05d78525

tombuildsstuff · 2018-05-22T15:31:11Z

aws/resource_aws_glue_trigger.go

+				Type:         schema.TypeString,
+				Required:     true,
+				ForceNew:     true,
+				ValidateFunc: validation.NoZeroValues,


off-topic: it feels like this should be set by default in TF Core when a fields required?

minor we generally sort Required -> Optional -> Computed in the schema

it feels like this should be set by default in TF Core when a fields required?

Some resource attributes (not here) use "" to denote removing or zero-ing out something currently. When Terraform 0.12 is released, maybe we can become more strict, but for now the validation is required.

we generally sort Required -> Optional -> Computed in the schema

Yikes, there are probably hundreds of AWS resources that need to be updated then. It would be nice if there was documentation or linting for this.

tombuildsstuff · 2018-05-22T15:31:25Z

aws/resource_aws_glue_trigger.go

+		},
+		Timeouts: &schema.ResourceTimeout{
+			Create: schema.DefaultTimeout(5 * time.Minute),
+			Delete: schema.DefaultTimeout(5 * time.Minute),


what about updating?

Currently the resource does not wait for updates so there's no timeout to configure. Acceptance testing so far has not triggered any issues with the enabled attribute on import which seems to imply updates are happening instantly (for now).

tombuildsstuff · 2018-05-22T15:31:55Z

aws/resource_aws_glue_trigger.go

+						"conditions": {
+							Type:     schema.TypeList,
+							Required: true,
+							MinItems: 1,


tombuildsstuff · 2018-05-22T15:33:01Z

aws/resource_aws_glue_trigger.go

+	log.Printf("[DEBUG] Reading Glue Trigger: %s", input)
+	output, err := conn.GetTrigger(input)
+	if err != nil {
+		if isAWSErr(err, glue.ErrCodeEntityNotFoundException, "") {


🤔 are we missing a generic "is not found" function? we added this in the AzureRM Provider to work around crashes when the connection dropped

Luckily, we don't get crashes from the SDK in these situations. The SDK will bubble up some sort of networking error (I believe always RequestError), which is related to handling like this I just added: https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/config.go#L354-L372

As for a generic "is not found" function -- all the AWS service APIs return something different in this regard (status codes and error codes, some services have error codes for each different resource as well), so unfortunately it would probably be more trouble than its worth to try and create a single function to handle that.

tombuildsstuff · 2018-05-22T16:25:31Z

aws/resource_aws_glue_trigger.go

+func flattenGlueActions(actions []*glue.Action) []interface{} {
+	l := []interface{}{}
+
+	for _, action := range actions {


can we add some crash protection here incase actions is nil?

I do not believe anything is required here: https://play.golang.org/p/-ShUuLbE9cc

tombuildsstuff · 2018-05-22T16:25:35Z

aws/resource_aws_glue_trigger.go

+func flattenGlueConditions(conditions []*glue.Condition) []interface{} {
+	l := []interface{}{}
+
+	for _, condition := range conditions {


(same here)

tombuildsstuff · 2018-05-22T16:41:34Z

aws/resource_aws_glue_trigger_test.go

+
+	input := &glue.GetTriggersInput{}
+	err = conn.GetTriggersPages(input, func(page *glue.GetTriggersOutput, lastPage bool) bool {
+		if len(page.Triggers) == 0 {


what if page is nil though?

Good catch -- will update 👍

tombuildsstuff · 2018-05-22T16:42:24Z

website/docs/r/glue_trigger.html.markdown

+
+# aws_glue_trigger
+
+Provides a Glue Job resource.


minor Provides -> Manages

tombuildsstuff · 2018-05-22T16:42:50Z

website/docs/r/glue_trigger.html.markdown

+Glue Triggers can be imported using `name`, e.g.
+
+```
+$ terraform import aws_glue_trigger.MyTrigger Myrigger


Myrigger -> MyTrigger

bflad · 2018-05-22T18:23:58Z

Acceptance testing still passes after resource code update -- merging!

make testacc TEST=./aws TESTARGS='-run=TestAccAWSGlueTrigger'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSGlueTrigger -timeout 120m
=== RUN   TestAccAWSGlueTrigger_Basic
--- PASS: TestAccAWSGlueTrigger_Basic (18.29s)
=== RUN   TestAccAWSGlueTrigger_Description
--- PASS: TestAccAWSGlueTrigger_Description (25.88s)
=== RUN   TestAccAWSGlueTrigger_Enabled
--- PASS: TestAccAWSGlueTrigger_Enabled (71.09s)
=== RUN   TestAccAWSGlueTrigger_Predicate
--- PASS: TestAccAWSGlueTrigger_Predicate (60.66s)
=== RUN   TestAccAWSGlueTrigger_Schedule
--- PASS: TestAccAWSGlueTrigger_Schedule (61.78s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	237.741s

bflad · 2018-05-23T21:29:32Z

This has been released in version 1.20.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-05T17:54:41Z

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!

New Resource: aws_glue_trigger

6ca7d33

bflad added new-resource Introduces a new resource. service/glue Issues and PRs that pertain to the glue service. labels May 6, 2018

bflad added this to the v1.18.0 milestone May 6, 2018

bflad requested a review from a team May 6, 2018 19:30

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

bflad mentioned this pull request May 6, 2018

Feature Request: Resource for managing AWS Glue Triggers #3882

Closed

bflad modified the milestones: v1.18.0, v1.19.0 May 10, 2018

bflad modified the milestones: v1.19.0, v1.20.0 May 17, 2018

tombuildsstuff approved these changes May 22, 2018

View reviewed changes

resource/aws_glue_trigger: Address #4464 PR feedback

a82e83b

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

bflad merged commit 6aeeb4d into master May 22, 2018

bflad deleted the f-aws_glue_trigger branch May 22, 2018 18:24

bflad added a commit that referenced this pull request May 22, 2018

Update CHANGELOG for #4464

7f7e9cc

ghost locked and limited conversation to collaborators Apr 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_glue_trigger #4464

New Resource: aws_glue_trigger #4464

bflad commented May 6, 2018

tombuildsstuff left a comment

tombuildsstuff May 22, 2018

bflad May 22, 2018

tombuildsstuff May 22, 2018

tombuildsstuff May 22, 2018

bflad May 22, 2018

tombuildsstuff May 22, 2018

bflad May 22, 2018

tombuildsstuff May 22, 2018

tombuildsstuff May 22, 2018

bflad May 22, 2018

tombuildsstuff May 22, 2018

bflad May 22, 2018

tombuildsstuff May 22, 2018

tombuildsstuff May 22, 2018

bflad May 22, 2018

tombuildsstuff May 22, 2018

bflad May 22, 2018

tombuildsstuff May 22, 2018

bflad May 22, 2018

bflad commented May 22, 2018

bflad commented May 23, 2018

ghost commented Apr 5, 2020

New Resource: aws_glue_trigger #4464

New Resource: aws_glue_trigger #4464

Conversation

bflad commented May 6, 2018

tombuildsstuff 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

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 May 22, 2018

bflad commented May 23, 2018

ghost commented Apr 5, 2020