-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for AWS Batch job definition timeout #4386
Add support for AWS Batch job definition timeout #4386
Conversation
Update the existing `aws_batch_job_definition` resource so that it supports the newly added timeout feature. The timeout configuration is supplied through a `timeout` block inside `aws_batch_job_definition` with a single `attempt_duration_seconds` attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @hectcastro -- this is looking pretty good, can you see the below and let us know if you have any questions?
@@ -123,6 +141,7 @@ func resourceAwsBatchJobDefinitionRead(d *schema.ResourceData, meta interface{}) | |||
d.Set("container_properties", job.ContainerProperties) | |||
d.Set("parameters", aws.StringValueMap(job.Parameters)) | |||
d.Set("retry_strategy", flattenRetryStrategy(job.RetryStrategy)) | |||
d.Set("timeout", flattenTimeout(job.Timeout)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When setting non-scalar values into the Terraform state, we should perform error checking:
if err := d.Set("timeout", flattenTimeout(job.Timeout)); err != nil {
return fmt.Errorf("error setting timeout: %s", err)
}
I'm pretty sure in this case you would have discovered an error about trying to set an *int64
into a TypeInt
, which was being silently ignored without the error check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. retry_strategy
is non-scalar too, so I added error checking to that as well.
} | ||
} | ||
|
||
func flattenTimeout(item *batch.JobTimeout) []map[string]interface{} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick: Since we use a very flat Go package structure in this repository currently, I'd recommend renaming this to flattenBatchJobTimeout
or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to flattenBatchJobTimeout
. Also, renamed flattenRetryStrategy
to flattenBatchRetryStrategy
to match.
data := []map[string]interface{}{} | ||
if item != nil { | ||
data = append(data, map[string]interface{}{ | ||
"attempt_duration_seconds": item.AttemptDurationSeconds, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this needs to be "attempt_duration_seconds": int(aws.Int64Value(item.AttemptDurationSeconds)),
-- the d.Set()
error checking should confirm this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated attempt_duration_seconds
and attempts
to match.
"attempt_duration_seconds": { | ||
Type: schema.TypeInt, | ||
Required: true, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the minimum accepted value is 60
we can add plan time validation via: ValidateFunc: validation.IntAtLeast(60),
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Thanks for pointing that out.
Schema: map[string]*schema.Schema{ | ||
"attempt_duration_seconds": { | ||
Type: schema.TypeInt, | ||
Required: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the AWS documentation, this is an optional field, see https://docs.aws.amazon.com/batch/latest/APIReference/API_JobTimeout.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, attempt_duration_seconds
is the lone attribute of the parent timeout
block. timeout
is optional, but attempt_duration_seconds
is not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, both timeout and attempt_duration_seconds are optional. I guess the idea is that at some point in the future AWS might add more ways to specify a timeout (eg. attempt_duration_minutes), in which case the individual fields can not be required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review, @bflad. I attempted to address all of your comments.
"attempt_duration_seconds": { | ||
Type: schema.TypeInt, | ||
Required: true, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Thanks for pointing that out.
Schema: map[string]*schema.Schema{ | ||
"attempt_duration_seconds": { | ||
Type: schema.TypeInt, | ||
Required: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, attempt_duration_seconds
is the lone attribute of the parent timeout
block. timeout
is optional, but attempt_duration_seconds
is not.
@@ -123,6 +141,7 @@ func resourceAwsBatchJobDefinitionRead(d *schema.ResourceData, meta interface{}) | |||
d.Set("container_properties", job.ContainerProperties) | |||
d.Set("parameters", aws.StringValueMap(job.Parameters)) | |||
d.Set("retry_strategy", flattenRetryStrategy(job.RetryStrategy)) | |||
d.Set("timeout", flattenTimeout(job.Timeout)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. retry_strategy
is non-scalar too, so I added error checking to that as well.
} | ||
} | ||
|
||
func flattenTimeout(item *batch.JobTimeout) []map[string]interface{} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to flattenBatchJobTimeout
. Also, renamed flattenRetryStrategy
to flattenBatchRetryStrategy
to match.
data := []map[string]interface{}{} | ||
if item != nil { | ||
data = append(data, map[string]interface{}{ | ||
"attempt_duration_seconds": item.AttemptDurationSeconds, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated attempt_duration_seconds
and attempts
to match.
Also, add validation to `attempts`.
For timeout and retry strategy.
|
||
`timeout` supports the following: | ||
|
||
* `attempt_duration_seconds` - (Required) The time duration in seconds after which AWS Batch terminates your jobs if they have not finished. The minimum value for the timeout is `60` seconds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still says Required
(and likewise for attempts
above). Both are now Optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let @bflad approve, but looks great! Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! 🚀
2 tests passed (all tests)
=== RUN TestAccAWSBatchJobDefinition_basic
--- PASS: TestAccAWSBatchJobDefinition_basic (6.53s)
=== RUN TestAccAWSBatchJobDefinition_updateForcesNewResource
--- PASS: TestAccAWSBatchJobDefinition_updateForcesNewResource (12.04s)
This has been released in version 1.17.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
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! |
Update the existing
aws_batch_job_definition
resource so that it supports the newly added timeout feature. The timeout configuration is supplied through atimeout
block insideaws_batch_job_definition
with a singleattempt_duration_seconds
attribute.This was tested via
make test
and by building aterraform-provider-aws
binary. The binary was used with an existing Terraform project aftertimeout {}
blocks were added to the existingaws_batch_job_definition
resources.Fixes #4186
See: