From d1196bb54fe9a0c01c1c3c36b380306176516210 Mon Sep 17 00:00:00 2001 From: tombuildsstuff Date: Wed, 6 Mar 2019 16:49:58 +0100 Subject: [PATCH] fixing issues raised in code review --- azurerm/resource_arm_container_group.go | 190 +++++++++++-------- azurerm/resource_arm_container_group_test.go | 49 ++--- website/docs/r/container_group.html.markdown | 58 +++--- 3 files changed, 169 insertions(+), 128 deletions(-) diff --git a/azurerm/resource_arm_container_group.go b/azurerm/resource_arm_container_group.go index 1613b8da9331d..e56437705f8a5 100644 --- a/azurerm/resource_arm_container_group.go +++ b/azurerm/resource_arm_container_group.go @@ -18,10 +18,6 @@ import ( "github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils" ) -const ( - LogAnalyticsWorkSpaceKeyPath = "log_analytics.0.workspace_key" -) - func resourceArmContainerGroup() *schema.Resource { return &schema.Resource{ Create: resourceArmContainerGroupCreate, @@ -283,41 +279,54 @@ func resourceArmContainerGroup() *schema.Resource { }, }, - "log_analytics": { + "diagnostics": { Type: schema.TypeList, Optional: true, ForceNew: true, MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ - "workspace_id": { - Type: schema.TypeString, - Required: true, - ForceNew: true, - ValidateFunc: validate.UUID, - }, - "workspace_key": { - Type: schema.TypeString, - Required: true, - Sensitive: true, - ForceNew: true, - ValidateFunc: validate.NoEmptyStrings, - }, - "log_type": { - Type: schema.TypeString, + "log_analytics": { + Type: schema.TypeList, Required: true, ForceNew: true, - ValidateFunc: validation.StringInSlice([]string{ - string(containerinstance.ContainerInsights), - string(containerinstance.ContainerInstanceLogs), - }, false), - }, - "metadata": { - Type: schema.TypeMap, - Optional: true, - ForceNew: true, - Elem: &schema.Schema{ - Type: schema.TypeString, + MaxItems: 1, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "workspace_id": { + Type: schema.TypeString, + Required: true, + ForceNew: true, + ValidateFunc: validate.UUID, + }, + + "workspace_key": { + Type: schema.TypeString, + Required: true, + Sensitive: true, + ForceNew: true, + ValidateFunc: validate.NoEmptyStrings, + }, + + "log_type": { + Type: schema.TypeString, + Required: true, + ForceNew: true, + ValidateFunc: validation.StringInSlice([]string{ + string(containerinstance.ContainerInsights), + string(containerinstance.ContainerInstanceLogs), + }, false), + }, + + "metadata": { + Type: schema.TypeMap, + Optional: true, + ForceNew: true, + Elem: &schema.Schema{ + Type: schema.TypeString, + }, + }, + }, }, }, }, @@ -363,6 +372,9 @@ func resourceArmContainerGroupCreate(d *schema.ResourceData, meta interface{}) e tags := d.Get("tags").(map[string]interface{}) restartPolicy := d.Get("restart_policy").(string) + diagnosticsRaw := d.Get("diagnostics").([]interface{}) + diagnostics := expandContainerGroupDiagnostics(diagnosticsRaw) + containers, containerGroupPorts, containerGroupVolumes := expandContainerGroupContainers(d) containerGroup := containerinstance.ContainerGroup{ Name: &name, @@ -370,6 +382,7 @@ func resourceArmContainerGroupCreate(d *schema.ResourceData, meta interface{}) e Tags: expandTags(tags), ContainerGroupProperties: &containerinstance.ContainerGroupProperties{ Containers: containers, + Diagnostics: diagnostics, RestartPolicy: containerinstance.ContainerGroupRestartPolicy(restartPolicy), IPAddress: &containerinstance.IPAddress{ Type: containerinstance.ContainerGroupIPAddressType(IPAddressType), @@ -385,11 +398,6 @@ func resourceArmContainerGroupCreate(d *schema.ResourceData, meta interface{}) e containerGroup.ContainerGroupProperties.IPAddress.DNSNameLabel = &dnsNameLabel } - logList := d.Get("log_analytics").([]interface{}) - containerGroup.Diagnostics = &containerinstance.ContainerGroupDiagnostics{ - LogAnalytics: expandContainerLogAnalytics(logList), - } - future, err := client.CreateOrUpdate(ctx, resGroup, name, containerGroup) if err != nil { return fmt.Errorf("Error creating/updating container group %q (Resource Group %q): %+v", name, resGroup, err) @@ -449,7 +457,7 @@ func resourceArmContainerGroupRead(d *schema.ResourceData, meta interface{}) err } if err := d.Set("image_registry_credential", flattenContainerImageRegistryCredentials(d, props.ImageRegistryCredentials)); err != nil { - return fmt.Errorf("Error setting `capabilities`: %+v", err) + return fmt.Errorf("Error setting `image_registry_credential`: %+v", err) } if address := props.IPAddress; address != nil { @@ -462,10 +470,8 @@ func resourceArmContainerGroupRead(d *schema.ResourceData, meta interface{}) err d.Set("restart_policy", string(props.RestartPolicy)) d.Set("os_type", string(props.OsType)) - if diag := props.Diagnostics; diag != nil { - if err := d.Set("log_analytics", flattenContainerLogAnalytics(d, diag.LogAnalytics)); err != nil { - return fmt.Errorf("Error setting `log_analytics`: %+v", err) - } + if err := d.Set("diagnostics", flattenContainerGroupDiagnostics(d, props.Diagnostics)); err != nil { + return fmt.Errorf("Error setting `diagnostics`: %+v", err) } } @@ -709,30 +715,6 @@ func expandContainerVolumes(input interface{}) (*[]containerinstance.VolumeMount return &volumeMounts, &containerGroupVolumes } -func expandContainerLogAnalytics(logList []interface{}) *containerinstance.LogAnalytics { - if len(logList) <= 0 { - return nil - } - - log := logList[0].(map[string]interface{}) - ws_id := log["workspace_id"].(string) - ws_key := log["workspace_key"].(string) - log_type := log["log_type"].(string) - metadataMap := log["metadata"].(map[string]interface{}) - metadata := make(map[string]*string) - for k, v := range metadataMap { - strValue := v.(string) - metadata[k] = &strValue - } - - return &containerinstance.LogAnalytics{ - WorkspaceID: &ws_id, - WorkspaceKey: &ws_key, - LogType: containerinstance.LogAnalyticsLogType(log_type), - Metadata: metadata, - } -} - func flattenContainerImageRegistryCredentials(d *schema.ResourceData, input *[]containerinstance.ImageRegistryCredential) []interface{} { if input == nil { return nil @@ -962,30 +944,80 @@ func flattenContainerVolumes(volumeMounts *[]containerinstance.VolumeMount, cont return volumeConfigs } -func flattenContainerLogAnalytics(d *schema.ResourceData, input *containerinstance.LogAnalytics) []interface{} { - if input == nil { - return []interface{}{} +func expandContainerGroupDiagnostics(input []interface{}) *containerinstance.ContainerGroupDiagnostics { + if len(input) == 0 { + return nil } - log := make(map[string]interface{}) + vs := input[0].(map[string]interface{}) + + analyticsVs := vs["log_analytics"].([]interface{}) + analyticsV := analyticsVs[0].(map[string]interface{}) + + workspaceId := analyticsV["workspace_id"].(string) + workspaceKey := analyticsV["workspace_key"].(string) + logType := containerinstance.LogAnalyticsLogType(analyticsV["log_type"].(string)) + + metadataMap := analyticsV["metadata"].(map[string]interface{}) + metadata := make(map[string]*string) + for k, v := range metadataMap { + strValue := v.(string) + metadata[k] = &strValue + } - if input.WorkspaceID != nil { - log["workspace_id"] = *input.WorkspaceID + return &containerinstance.ContainerGroupDiagnostics{ + LogAnalytics: &containerinstance.LogAnalytics{ + WorkspaceID: utils.String(workspaceId), + WorkspaceKey: utils.String(workspaceKey), + LogType: logType, + Metadata: metadata, + }, } +} - if v, ok := d.GetOk(LogAnalyticsWorkSpaceKeyPath); ok { - log["workspace_key"] = v +func flattenContainerGroupDiagnostics(d *schema.ResourceData, input *containerinstance.ContainerGroupDiagnostics) []interface{} { + if input == nil { + return []interface{}{} } - log["log_type"] = input.LogType + logAnalytics := make([]interface{}, 0) + + if la := input.LogAnalytics; la != nil { + output := make(map[string]interface{}, 0) + + output["log_type"] = string(la.LogType) - metadata := make(map[string]interface{}) - for k, v := range input.Metadata { - metadata[k] = *v + metadata := make(map[string]interface{}) + for k, v := range la.Metadata { + metadata[k] = *v + } + output["metadata"] = metadata + + if la.WorkspaceID != nil { + output["workspace_id"] = *la.WorkspaceID + } + + // the existing config may not exist at Import time, protect against it. + workspaceKey := "" + if existingDiags := d.Get("diagnostics").([]interface{}); len(existingDiags) > 0 { + existingDiag := existingDiags[0].(map[string]interface{}) + if existingLA := existingDiag["log_analytics"].([]interface{}); len(existingLA) > 0 { + vs := existingLA[0].(map[string]interface{}) + if key := vs["workspace_key"]; key != nil && key.(string) != "" { + workspaceKey = key.(string) + } + } + } + output["workspace_key"] = workspaceKey + + logAnalytics = append(logAnalytics, output) } - log["metadata"] = metadata - return []interface{}{log} + return []interface{}{ + map[string]interface{}{ + "log_analytics": logAnalytics, + }, + } } func resourceArmContainerGroupPortsHash(v interface{}) int { diff --git a/azurerm/resource_arm_container_group_test.go b/azurerm/resource_arm_container_group_test.go index e35c5ccb1349c..4833aaed93cba 100644 --- a/azurerm/resource_arm_container_group_test.go +++ b/azurerm/resource_arm_container_group_test.go @@ -5,7 +5,6 @@ import ( "net/http" "testing" - "github.com/Azure/azure-sdk-for-go/services/containerinstance/mgmt/2018-10-01/containerinstance" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/terraform" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/tf" @@ -218,11 +217,11 @@ func TestAccAzureRMContainerGroup_linuxComplete(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "container.0.volume.0.read_only", "false"), resource.TestCheckResourceAttr(resourceName, "os_type", "Linux"), resource.TestCheckResourceAttr(resourceName, "restart_policy", "OnFailure"), - resource.TestCheckResourceAttr(resourceName, "log_analytics.#", "1"), - resource.TestCheckResourceAttr(resourceName, "log_analytics.0.log_type", string(containerinstance.ContainerInsights)), - resource.TestCheckResourceAttr(resourceName, "log_analytics.0.metadata.%", "1"), - resource.TestCheckResourceAttrSet(resourceName, "log_analytics.0.workspace_id"), - resource.TestCheckResourceAttrSet(resourceName, "log_analytics.0.workspace_key"), + resource.TestCheckResourceAttr(resourceName, "diagnostics.0.log_analytics.#", "1"), + resource.TestCheckResourceAttr(resourceName, "diagnostics.0.log_analytics.0.log_type", "ContainerInsights"), + resource.TestCheckResourceAttr(resourceName, "diagnostics.0.log_analytics.0.metadata.%", "1"), + resource.TestCheckResourceAttrSet(resourceName, "diagnostics.0.log_analytics.0.workspace_id"), + resource.TestCheckResourceAttrSet(resourceName, "diagnostics.0.log_analytics.0.workspace_key"), ), }, { @@ -300,11 +299,11 @@ func TestAccAzureRMContainerGroup_windowsComplete(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "container.0.secure_environment_variables.secureFoo1", "secureBar1"), resource.TestCheckResourceAttr(resourceName, "os_type", "Windows"), resource.TestCheckResourceAttr(resourceName, "restart_policy", "Never"), - resource.TestCheckResourceAttr(resourceName, "log_analytics.#", "1"), - resource.TestCheckResourceAttr(resourceName, "log_analytics.0.log_type", string(containerinstance.ContainerInsights)), - resource.TestCheckResourceAttr(resourceName, "log_analytics.0.metadata.%", "1"), - resource.TestCheckResourceAttrSet(resourceName, "log_analytics.0.workspace_id"), - resource.TestCheckResourceAttrSet(resourceName, "log_analytics.0.workspace_key"), + resource.TestCheckResourceAttr(resourceName, "diagnostics.0.log_analytics.#", "1"), + resource.TestCheckResourceAttr(resourceName, "diagnostics.0.log_analytics.0.log_type", "ContainerInsights"), + resource.TestCheckResourceAttr(resourceName, "diagnostics.0.log_analytics.0.metadata.%", "1"), + resource.TestCheckResourceAttrSet(resourceName, "diagnostics.0.log_analytics.0.workspace_id"), + resource.TestCheckResourceAttrSet(resourceName, "diagnostics.0.log_analytics.0.workspace_key"), ), }, { @@ -315,7 +314,7 @@ func TestAccAzureRMContainerGroup_windowsComplete(t *testing.T) { "container.0.secure_environment_variables.%", "container.0.secure_environment_variables.secureFoo", "container.0.secure_environment_variables.secureFoo1", - "log_analytics.0.workspace_key", + "diagnostics.0.log_analytics.0.workspace_key", }, }, }, @@ -608,12 +607,14 @@ resource "azurerm_container_group" "test" { commands = ["cmd.exe", "echo", "hi"] } - log_analytics { - workspace_id = "${azurerm_log_analytics_workspace.test.workspace_id}" - workspace_key = "${azurerm_log_analytics_workspace.test.primary_shared_key}" - log_type = "ContainerInsights" - metadata { - "node-name" = "acctestContainerGroup" + diagnostics { + log_analytics { + workspace_id = "${azurerm_log_analytics_workspace.test.workspace_id}" + workspace_key = "${azurerm_log_analytics_workspace.test.primary_shared_key}" + log_type = "ContainerInsights" + metadata { + "node-name" = "acctestContainerGroup" + } } } @@ -711,12 +712,14 @@ resource "azurerm_container_group" "test" { commands = ["/bin/bash", "-c", "ls"] } + diagnostics { log_analytics { - workspace_id = "${azurerm_log_analytics_workspace.test.workspace_id}" - workspace_key = "${azurerm_log_analytics_workspace.test.primary_shared_key}" - log_type = "ContainerInsights" - metadata { - "node-name" = "acctestContainerGroup" + workspace_id = "${azurerm_log_analytics_workspace.test.workspace_id}" + workspace_key = "${azurerm_log_analytics_workspace.test.primary_shared_key}" + log_type = "ContainerInsights" + metadata { + "node-name" = "acctestContainerGroup" + } } } diff --git a/website/docs/r/container_group.html.markdown b/website/docs/r/container_group.html.markdown index 2fc1f45a407f9..fa66278635225 100644 --- a/website/docs/r/container_group.html.markdown +++ b/website/docs/r/container_group.html.markdown @@ -102,27 +102,29 @@ The following arguments are supported: * `location` - (Required) Specifies the supported Azure location where the resource exists. Changing this forces a new resource to be created. -* `ip_address_type` - (Optional) Specifies the ip address type of the container. `Public` is the only acceptable value at this time. Changing this forces a new resource to be created. +* `container` - (Required) The definition of a container that is part of the group as documented in the `container` block below. Changing this forces a new resource to be created. -* `dns_name_label` - (Optional) The DNS label/name for the container groups IP. +~> **Note:** if `os_type` is set to `Windows` currently only a single `container` block is supported. * `os_type` - (Required) The OS for the container group. Allowed values are `Linux` and `Windows`. Changing this forces a new resource to be created. -* `restart_policy` - (Optional) Restart policy for the container group. Allowed values are `Always`, `Never`, `OnFailure`. Defaults to `Always`. +--- -* `image_registry_credential` - (Optional) Set image registry credentials for the group as documented in the `image_registry_credential` block below. +* `diagnostics` - (Optional) A `diagnostics` block as documented below. -* `container` - (Required) The definition of a container that is part of the group as documented in the `container` block below. Changing this forces a new resource to be created. +* `dns_name_label` - (Optional) The DNS label/name for the container groups IP. -* `log_analytics` - (Optional) The information of Log Analytics for the group as documented in the `log_analytics` block below. Changing this forces a new resource to be created. +* `ip_address_type` - (Optional) Specifies the ip address type of the container. `Public` is the only acceptable value at this time. Changing this forces a new resource to be created. -~> **Note:** if `os_type` is set to `Windows` currently only a single `container` block is supported. +* `image_registry_credential` - (Optional) A `image_registry_credential` block as documented below. + +* `restart_policy` - (Optional) Restart policy for the container group. Allowed values are `Always`, `Never`, `OnFailure`. Defaults to `Always`. * `tags` - (Optional) A mapping of tags to assign to the resource. --- -The `container` block supports: +A `container` block supports: * `name` - (Required) Specifies the name of the Container. Changing this forces a new resource to be created. @@ -148,33 +150,35 @@ The `container` block supports: --- -The `volume` block supports: +A `diagnostics` block supports: -* `name` - (Required) The name of the volume mount. Changing this forces a new resource to be created. +* `log_analytics` - (Required) A `log_analytics` block as defined below. -* `mount_path` - (Required) The path on which this volume is to be mounted. Changing this forces a new resource to be created. +--- -* `read_only` - (Optional) Specify if the volume is to be mounted as read only or not. The default value is `false`. Changing this forces a new resource to be created. +A `image_registry_credential` block supports: -* `storage_account_name` - (Required) The Azure storage account from which the volume is to be mounted. Changing this forces a new resource to be created. +* `username` - (Required) The username with which to connect to the registry. -* `storage_account_key` - (Required) The access key for the Azure Storage account specified as above. Changing this forces a new resource to be created. +* `password` - (Required) The password with which to connect to the registry. -* `share_name` - (Required) The Azure storage share that is to be mounted as a volume. This must be created on the storage account specified as above. Changing this forces a new resource to be created. +* `server` - (Required) The address to use to connect to the registry without protocol ("https"/"http"). For example: "myacr.acr.io" --- -The `image_registry_credential` block supports: +A `log_analytics` block supports: -* `username` - (Required) The username with which to connect to the registry. +* `log_type` - (Required) The log type which should be used. Possible values are `ContainerInsights` and `ContainerInstanceLogs`. -* `password` - (Required) The password with which to connect to the registry. +* `workspace_id` - (Required) The Workspace ID of the Log Analytics Workspace. -* `server` - (Required) The address to use to connect to the registry without protocol ("https"/"http"). For example: "myacr.acr.io" +* `workspace_key` - (Required) The Workspace Key of the Log Analytics Workspace. + +* `metadata` - (Optional) Any metadata required for Log Analytics. --- -The `ports` block supports: +A `ports` block supports: * `port` - (Required) The port number the container will expose. @@ -182,17 +186,19 @@ The `ports` block supports: --- -The `log_analytics` block supports: +A `volume` block supports: + +* `name` - (Required) The name of the volume mount. Changing this forces a new resource to be created. -* `workspace_id` - (Required) The workspace id for Log Analytics. +* `mount_path` - (Required) The path on which this volume is to be mounted. Changing this forces a new resource to be created. -~> **NOTE:** This field is `workspace_id` of `azurerm_log_analytics_workspace.test.workspace_id`, NOT `id`. +* `read_only` - (Optional) Specify if the volume is to be mounted as read only or not. The default value is `false`. Changing this forces a new resource to be created. -* `workspace_key` - (Required) The workspace key for Log Analytics. +* `storage_account_name` - (Required) The Azure storage account from which the volume is to be mounted. Changing this forces a new resource to be created. -* `log_type` - (Required) The log type to be used. Possible values include: `ContainerInsights` and `ContainerInstanceLogs`. +* `storage_account_key` - (Required) The access key for the Azure Storage account specified as above. Changing this forces a new resource to be created. -* `metadata` - (Optional) The metadata for Log Analytics. +* `share_name` - (Required) The Azure storage share that is to be mounted as a volume. This must be created on the storage account specified as above. Changing this forces a new resource to be created. ## Attributes Reference