IC for adding container_configuration to batch pool resource. #3311
Conversation
azurerm/helpers/azure/batch_pool.go
Outdated
| // ExpandBatchPoolContainerConfiguration expands the Batch pool container configuration | ||
| func ExpandBatchPoolContainerConfiguration(list []interface{}) (*batch.ContainerConfiguration, error) { | ||
| if len(list) == 0 { | ||
| return nil, fmt.Errorf("Error: container configuration should be defined") |
There was a problem hiding this comment.
We should just return nil here? i don't think this is an actual error
azurerm/resource_arm_batch_pool.go
Outdated
| @@ -567,6 +588,15 @@ func resourceArmBatchPoolRead(d *schema.ResourceData, meta interface{}) error { | |||
| d.Set("node_agent_sku_id", props.DeploymentConfiguration.VirtualMachineConfiguration.NodeAgentSkuID) | |||
| } | |||
|
|
|||
| if props.DeploymentConfiguration != nil && | |||
There was a problem hiding this comment.
Could we change this to:
if dcfg := props.DeploymentConfiguration; dcfg != nil
if vmcfg := dcfg.VirtualMachineConfiguration; vmcfg != nil {
d.Set("container_configuration", azure.FlattenBatchPoolContainerConfiguration(vmcfg. ContainerConfiguration)) //this function should check if its nil and return nothing if it is
azurerm/data_source_batch_pool.go
Outdated
| @@ -274,6 +288,17 @@ func dataSourceArmBatchPoolRead(d *schema.ResourceData, meta interface{}) error | |||
| } | |||
| } | |||
|
|
|||
| if props.DeploymentConfiguration != nil && | |||
There was a problem hiding this comment.
Same comment as below in the resource. we should nest a bunch of assignments and if's
There was a problem hiding this comment.
Cool, thanks for the comment, looks much batter. I made the update and slightly refactored the later code since it also uses dcfg.VirtualMachineConfiguration.
|
@katbyte Thanks for the review! I tried to address the comments, let me know what you think. |
azurerm/helpers/azure/batch_pool.go
Outdated
| result["type"] = *armContainerConfiguration.Type | ||
| } | ||
|
|
||
| return result |
There was a problem hiding this comment.
I think this needs to be:
| return result | |
| return []interface{}{result} |
to fix:
------- Stdout: -------
=== RUN TestAccDataSourceAzureRMBatchPool_complete
=== PAUSE TestAccDataSourceAzureRMBatchPool_complete
=== CONT TestAccDataSourceAzureRMBatchPool_complete
------- Stderr: -------
panic: container_configuration: '': source data must be an array or slice, got map
| @@ -128,6 +132,8 @@ The following arguments are supported: | |||
|
|
|||
| * `certificate` - (Optional) One or more `certificate` blocks that describe the certificates to be installed on each compute node in the pool. | |||
|
|
|||
| * `container_configuration` - (Optional) The container configuration used in the pool's VMs. | |||
There was a problem hiding this comment.
Looks like your missing the type attribute description here
|
@katbyte Thanks for the comments! I think I've made the changes in the last two commits: https://github.com/terraform-providers/terraform-provider-azurerm/compare/75879f6..f925fdc. I rebased off master as I was getting some errors I think were resolved later commits. |
|
This has been released in version 1.28.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 1.28.0"
}
# ... other configuration ... |
|
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 👉 [email protected]. Thanks! |
ghost
locked and limited conversation to collaborators
Hi -- this PR adds the
container_configurationblock to the batch pool resource.This setting is necessary for the Pool to support containerized workloads. There are more settings not included in this PR, e.g. setting a default registry. I'd like to add that at some point, but this seemed like a good intermediate step since this is my first PR and if the docker is enabled then the Tasks can still set the registry or other items.