Add max_tasks_per_node setting to Batch Pool #2805
Conversation
stuartleeks
changed the title
There was a problem hiding this comment.
@stuartleeks Thanks for the PR. I've gone through all the changes here, most of them LGTM. If you can address the comments in line, then it's good for merge.
azurerm/resource_arm_batch_pool.go
Outdated
| "max_tasks_per_node": { | ||
| Type: schema.TypeInt, | ||
| Optional: true, | ||
| Default: 1, |
There was a problem hiding this comment.
Can you add a validation for this field ?
azurerm/resource_arm_batch_pool.go
Outdated
| @@ -243,6 +248,8 @@ func resourceArmBatchPoolCreate(d *schema.ResourceData, meta interface{}) error | |||
| poolName := d.Get("name").(string) | |||
| displayName := d.Get("display_name").(string) | |||
| vmSize := d.Get("vm_size").(string) | |||
| maxTasksPerNode := d.Get("max_tasks_per_node").(int) | |||
There was a problem hiding this comment.
Can we directly assert it as int32 ?
There was a problem hiding this comment.
I've removed the temporary variable now!
azurerm/resource_arm_batch_pool.go
Outdated
| @@ -243,6 +248,8 @@ func resourceArmBatchPoolCreate(d *schema.ResourceData, meta interface{}) error | |||
| poolName := d.Get("name").(string) | |||
| displayName := d.Get("display_name").(string) | |||
| vmSize := d.Get("vm_size").(string) | |||
| maxTasksPerNode := d.Get("max_tasks_per_node").(int) | |||
| foo := int32(maxTasksPerNode) | |||
There was a problem hiding this comment.
Can we use a more readable name ? For example, maxTasks.
There was a problem hiding this comment.
Oops - I meant to use a clearer name here! Done in latest commits
azurerm/resource_arm_batch_pool.go
Outdated
| VMSize: &vmSize, | ||
| DisplayName: &displayName, | ||
| MaxTasksPerNode: &foo, | ||
| // MaxTasksPerNode: &maxTasksPerNode, |
There was a problem hiding this comment.
| // MaxTasksPerNode: &maxTasksPerNode, |
There was a problem hiding this comment.
Done in latest commits
|
@metacpp - Thanks for your review. I've just pushed some changes to address your feedback. Let me know if you have any further comments. |
There was a problem hiding this comment.
Hi @stuartleeks,
Thanks for the enhancement! for the most part this LGTM aside from a couple comments i've left inline. Once those are fixed up it should be good to merge 🙂
| @@ -102,6 +102,8 @@ The following arguments are supported: | |||
|
|
|||
| * `display_name` - (Optional) Specifies the display name of the Batch pool. | |||
|
|
|||
| * `max_tasks_per_node` - (Optional) Specifies the maximum number of tasks that can run concurrently on a single compute node in the pool. | |||
There was a problem hiding this comment.
Could we add the default value here?
| * `max_tasks_per_node` - (Optional) Specifies the maximum number of tasks that can run concurrently on a single compute node in the pool. | |
| * `max_tasks_per_node` - (Optional) Specifies the maximum number of tasks that can run concurrently on a single compute node in the pool. Defaults to `1`. |
There was a problem hiding this comment.
Yup - added to both docs
| @@ -323,7 +324,8 @@ resource "azurerm_batch_pool" "test" { | |||
| resource_group_name = "${azurerm_resource_group.test.name}" | |||
| account_name = "${azurerm_batch_account.test.name}" | |||
| display_name = "Test Acc Pool" | |||
| vm_size = "Standard_A1" | |||
| vm_size = "Standard_A1", | |||
There was a problem hiding this comment.
Could we fix the formatting/alignment here?
|
Thanks @katbyte - I've made the changes you requested.Let me know if you have any further comments |
Co-Authored-By: stuartleeks <stuart@leeks.net>
|
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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
ghost
locked and limited conversation to collaborators
No description provided.