ACI: Add support for Volume mounts #366
Conversation
Merge from upstream
…rm-provider-azurerm into terraform-providers-master Conflicts: website/docs/r/container_group.html.markdown
Merge from master
TBD: Password forces new resource, test
Merge from master
There was a problem hiding this comment.
Hey @abhijeetgaiha
Thanks for this PR - I've taken a look and have left a few comments but this mostly LGTM :)
Thanks!
| @@ -206,7 +253,7 @@ func resourceArmContainerGroupRead(d *schema.ResourceData, meta interface{}) err | |||
| d.Set("ip_address", address.IP) | |||
| } | |||
|
|
|||
| containerConfigs := flattenContainerGroupContainers(resp.Containers) | |||
| containerConfigs := flattenContainerGroupContainers(d, resp.Containers, resp.ContainerGroupProperties.IPAddress.Ports, resp.ContainerGroupProperties.Volumes) | |||
There was a problem hiding this comment.
if resp.ContainerGroupProperties is nil (e.g. Swagger/API spec changes), there's a possibility for a crash here - could we instead wrap this via:
if props := resp.ContainerGroupProperties; props != nil {
containerConfigs := flattenContainerGroupContainers(d, resp.Containers, props.IPAddress.Ports, props.Volumes)
# ...
}
| containerConfig["port"] = containerPort | ||
| // protocol isn't returned in container config, have to search in container group ports | ||
| protocol := "" | ||
| for _, cgPort := range *containerGroupPorts { |
There was a problem hiding this comment.
do we need a check to ensure this isn't nil?
| }, | ||
| } | ||
|
|
||
| *containerGroupVolumes = append(*containerGroupVolumes, cv) |
There was a problem hiding this comment.
I'd argue this would be clearer if we instead returned multiple objects from this method, rather than appending to the input argument i.e.
func foo(bar string) (*[]containerinstance.Volume, *[]containerinstance.VolumeMount) {
return volumes, volumeMounts
}
There was a problem hiding this comment.
It has to be this way since the containerGroupVolumes is a collection of all volumes across all containers.
There was a problem hiding this comment.
Reading the code above, it takes understanding of the inner working of the method to know that both a value passed in will be modified, and a return value will come out - in general methods tend to do one or the other (and I'd argue the latter is easier to understand). We should be able to achieve the same thing by returning the value and then setting it - for instance:
func foo() {
containerGroupVolumes := make([]containerinstance.Volume, 0)
for i := 0; i < 9; i++ {
mounts, containerGroupVolumes := expandContainerVolumes(interface{}, &containerGroupVolumes)
// mounts is local
// containerGroupVolumes is appended too
}
}
func expandContainerVolumes(input interface{}, containerGroupVolumes *[]containerinstance.Volume) (*[]containerinstance.VolumeMount, containerGroupVolumes *[]containerinstance.Volume) {
mounts := make([]containerinstance.VolumeMount, 0)
volumes := *containerGroupVolumes
volumes = append(volumes, ...)
return &mounts, &volumes
}
| tags { | ||
| environment = "testing" | ||
| } | ||
| } |
There was a problem hiding this comment.
really minor could we normalise the formatting this?
| @@ -86,6 +111,22 @@ The `container` block supports: | |||
|
|
|||
| * `command` - (Optional) A command line to be run on the container. Changing this forces a new resource to be created. | |||
|
|
|||
| * `volume` - (Optional) The definition of a volume mount for this container. Changing this forces a new resource to be created. | |||
There was a problem hiding this comment.
The definition of a volume mount for this container
could we suffix this with "as documented in the volume block below"?
There was a problem hiding this comment.
Thanks for the continued effort on this - I've taken another look through and this LGTM.
Tests pass:
$ acctests azurerm TestAccAzureRMContainerGroup
=== RUN TestAccAzureRMContainerGroup_linuxBasic
--- PASS: TestAccAzureRMContainerGroup_linuxBasic (62.53s)
=== RUN TestAccAzureRMContainerGroup_linuxBasicUpdate
--- PASS: TestAccAzureRMContainerGroup_linuxBasicUpdate (71.28s)
=== RUN TestAccAzureRMContainerGroup_linuxComplete
--- PASS: TestAccAzureRMContainerGroup_linuxComplete (88.78s)
=== RUN TestAccAzureRMContainerGroup_windowsBasic
--- PASS: TestAccAzureRMContainerGroup_windowsBasic (60.53s)
=== RUN TestAccAzureRMContainerGroup_windowsComplete
--- PASS: TestAccAzureRMContainerGroup_windowsComplete (58.66s)
PASS
ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 341.800s
Thanks!
|
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
Added support for Volume mounts in Azure container groups.