-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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 new data source aws_batch_job_queue #4288
Conversation
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 this @gheine -- left some initial comments below. Please let us know if you have any questions.
name = "%[1]s" | ||
state = "ENABLED" | ||
priority = 1 | ||
compute_environments = [] |
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.
A value is required inside this list:
--- FAIL: TestAccDataSourceAwsBatchJobQueue (4.31s)
testing.go:518: Step 0 error: Error applying: 2 error(s) occurred:
* aws_batch_job_queue.wrong: 1 error(s) occurred:
* aws_batch_job_queue.wrong: InvalidParameter: 1 validation error(s) found.
- missing required field, CreateJobQueueInput.ComputeEnvironmentOrder.
"tf_acc_test_-4822913165239202394_wrong"
* aws_batch_job_queue.test: 1 error(s) occurred:
* aws_batch_job_queue.test: InvalidParameter: 1 validation error(s) found.
- missing required field, CreateJobQueueInput.ComputeEnvironmentOrder.
"tf_acc_test_-4822913165239202394"
ceos := make([]map[string]interface{}, 0) | ||
for _, v := range jobQueue.ComputeEnvironmentOrder { | ||
ceo := map[string]interface{}{} | ||
ceo["compute_environment"] = *v.ComputeEnvironment |
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.
To prevent provider panics, we should opt to safely read these:
ceo["compute_environment"] = aws.StringValue(v.ComputeEnvironment),
ceo["order"] = int(aws.Int64Value(v.Order)),
ceo["order"] = *v.Order | ||
ceos = append(ceos, ceo) | ||
} | ||
d.Set("compute_environment_order", ceos) |
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 calling d.Set()
with non-scalar values, we should implement the error checking to ensure we're properly able to set the attribute into the Terraform state based on the schema:
if err := d.Set("compute_environment_order", ceos); err != nil {
return fmt.Errorf("error setting compute_environment_order: %s", err)
}
I can tell you from looking at the the existing aws_batch_job_queue
resource that it does not properly set compute_environments
into the Terraform state (and therefore cannot detect drift for that 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.
Thanks @gheine! LGTM (I'll fix the documentation issue on merge)
1 test passed (all tests)
=== RUN TestAccDataSourceAwsBatchJobQueue
--- PASS: TestAccDataSourceAwsBatchJobQueue (121.77s)
@@ -58,6 +58,8 @@ | |||
<li<%= sidebar_current("docs-aws-datasource-batch-compute-environment") %>> | |||
<a href="/docs/providers/aws/d/batch_compute_environment.html">aws_batch_compute_environment</a> | |||
</li> | |||
<li<%= sidebar_current("docs-aws-datasource-batch-job-queue") %>> | |||
<a href="/docs/providers/aws/d/batch_job_queue.html">aws_batch_job_queue</a> |
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.
Missing </li>
here
This has been released in version 1.16.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! |
No description provided.