Implementation of Azure container groups #311

abhijeetgaiha · 2017-09-07T02:01:20Z

This has a basic implementation for #287 .

The following features are TBD:

Command lines
Volume Mounts
Environment Variables
Private container registries

tombuildsstuff

Hey @abhijeetgaiha

Thanks for this PR :)

I've taken a look through and left some comments in-line, but this is off to a good start :)

Thanks!

tombuildsstuff · 2017-09-12T08:18:33Z

azurerm/resource_arm_container_group.go

+			"ip_address_type": {
+				Type:             schema.TypeString,
+				Required:         true,
+				DiffSuppressFunc: ignoreCaseDiffSuppressFunc,


I'm assuming there's a limited set of options here? if so - could we add some validation to this, e.g.

ValidateFunc: validation.StringInSlice([]string{ "foo", "bar", }, true)

(as below) we should be able to make this Optional and Default: "Public" given `Public" is the only option at this time, which would mean users don't need to specify it :)

tombuildsstuff · 2017-09-12T08:18:42Z

azurerm/resource_arm_container_group.go

+		Schema: map[string]*schema.Schema{
+			"name": {
+				Type:     schema.TypeString,
+				Required: true,


I think this also needs to be ForceNew?

tombuildsstuff · 2017-09-12T08:18:44Z

azurerm/resource_arm_container_group.go

+
+			"resource_group_name": {
+				Type:     schema.TypeString,
+				Required: true,


I think this also needs to be ForceNew?

tombuildsstuff · 2017-09-12T08:18:54Z

azurerm/resource_arm_container_group.go

+			"os_type": {
+				Type:             schema.TypeString,
+				Required:         true,
+				DiffSuppressFunc: ignoreCaseDiffSuppressFunc,


I'm assuming there's a limited set of options here? if so - could we add some validation to this, e.g.

ValidateFunc: validation.StringInSlice([]string{ "foo", "bar", }, true)

tombuildsstuff · 2017-09-12T08:19:54Z

azurerm/resource_arm_container_group.go

+	}
+
+	_, error := containerGroupsClient.CreateOrUpdate(resGroup, name, containerGroup)
+	if error != nil {


minor can we swap error out for err to match the other resources?

tombuildsstuff · 2017-09-12T08:29:17Z

website/docs/r/container_group.html.markdown

+}
+```
+
+## Example Usage (Windows)


given the only difference between these two is the OS field - I think we can just use the Linux example (and just rename that to Example Usage)?

I've updated the linux example to use two containers.

could we consolidate this into a single example, as they're fundamentally the same (compared to say Container Service in which some fields aren't valid for a non-Kubernetes cluster)?

you mean two container groups in one file?

I was referring to picking either the Windows or the Linux example and removing the other (since they're virtually the same)

tombuildsstuff · 2017-09-12T08:29:59Z

website/docs/r/container_group.html.markdown

+
+* `location` - (Required) Specifies the supported Azure location where the resource exists. Changing this forces a new resource to be created. Changing this forces a new resource to be created.
+
+* `ip_address_type` - (Required) 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.


If this is the only acceptable value at this time, we can make this field Optional and Default it to Public for the moment - so users don't have to specify it (in the schema)

tombuildsstuff · 2017-09-12T08:30:26Z

website/docs/r/container_group.html.markdown

+
+The following attributes are exported:
+
+* `ip_address` - The IP address allocated to the container group.


there's also the ID of the container group as id

tombuildsstuff · 2017-09-12T08:30:52Z

azurerm/resource_arm_container_group.go

+			"container": {
+				Type:     schema.TypeList,
+				Required: true,
+				Elem: &schema.Resource{


given only a single container's currently supported - can we add MaxItems: 1 here?

Actually, more than 1 container is supported for Linux. Updated docs.

tombuildsstuff · 2017-09-12T08:31:18Z

azurerm/resource_arm_container_group.go

+
+						"name": {
+							Type:     schema.TypeString,
+							Required: true,


these are marked as ForceNew in the documentation below - if so, can we add ForceNew: true to all of these fields in the container group?

I might have missed it, but I can't see these changes?

I fixed the docs. This shouldn't force the whole group to be updated.

tombuildsstuff · 2017-09-14T07:24:31Z

azurerm/resource_arm_container_group.go

+							ValidateFunc: validation.StringInSlice([]string{
+								"tcp",
+								"udp",
+							}, true),


given the validation function allows for differences in case (the true) can we add a DiffSuppressFunc to this to ignore the case?

tombuildsstuff · 2017-09-14T07:28:10Z

azurerm/resource_arm_container_group.go

+	IPAddressType := d.Get("ip_address_type").(string)
+	tags := d.Get("tags").(map[string]interface{})
+
+	containersConfig := d.Get("container").([]interface{})


can we pull all of this out into a separate function expandContainerGroupContainers which returns the deserialized object?

tombuildsstuff · 2017-09-14T07:29:02Z

azurerm/resource_arm_container_group.go

+			},
+		}
+
+		if port != 0 {


given this is an Optional field, we should be able to do this instead:

if v, ok := data["port"]; ok { ... }

tombuildsstuff · 2017-09-14T07:29:20Z

azurerm/resource_arm_container_group.go

+				Port: &port,
+			}
+
+			if protocol != "" {


(same as for port) for the Protocol field

tombuildsstuff · 2017-09-14T07:29:31Z

azurerm/resource_arm_container_group.go

+		// 		envVarsList = append(envVarsList, envVar)
+		// 	}
+		// 	container.EnvironmentVariables = &envVarsList
+		// }


can we remove this, since it's not being used?

tombuildsstuff · 2017-09-14T07:30:54Z

azurerm/resource_arm_container_group.go

+	_, err = containterGroupsClient.Delete(resGroup, name)
+
+	if err != nil {
+		return err


can we check for both a 404 and a dropped connection before throwing an error? we can do this via:

resp, err = containterGroupsClient.Delete(resGroup, name) if err != nil && !utils.ResponseWasNotFound(resp) { return nil } return nil

tombuildsstuff · 2017-09-14T07:32:13Z

azurerm/resource_arm_container_group_test.go

+resource "azurerm_container_group" "test" {
+
+	name = "acctestcontainergroup-%d"
+	location = "%s"


rather than threading this through in multiple places, can we update this to "${azurerm_resource_group.test.location}"?

tombuildsstuff · 2017-09-14T07:32:21Z

azurerm/resource_arm_container_group_test.go

+resource "azurerm_container_group" "test" {
+
+	name = "acctestcontainergroup-%d"
+	location = "%s"


rather than threading this through in multiple places, can we update this to "${azurerm_resource_group.test.location}"?

tombuildsstuff · 2017-09-14T07:34:40Z

examples/aci-linux-multi/main.tf

+
+resource "azurerm_container_group" "aci-test" {
+    name = "mynginx"
+    location = "west us"


minor since Location is available on the Resource Group resource, could we update this to "${azurerm_resource_group.aci-rg.location}" to keep it a little DRY-er?, rather than in each resource? (I'm slowly working through the documentation for each resource to show this)

tombuildsstuff · 2017-09-14T07:37:11Z

website/docs/r/container_group.html.markdown

+resource "azurerm_container_group" "winapp" {
+
+  name = "mywinapp"
+  location = "west us"


minor can we update this to be "${azurerm_resource_group.aci-rg.location}"

Removed the windows example.

tombuildsstuff · 2017-09-14T07:37:42Z

website/docs/r/container_group.html.markdown

+resource "azurerm_container_group" "aci-helloworld" {
+
+  name = "aci-hw"
+  location = "west us"


minor can we update this to be "${azurerm_resource_group.aci-rg.location}"

tombuildsstuff · 2017-09-14T07:40:21Z

website/docs/r/container_group.html.markdown

+
+* `os_type` - (Required) The OS for the container group. Allowed values are `linux` and `windows` Changing this forces a new resource to be created.
+
+* `container` - (Required) The definition of a container that is part of the group. Currently, only single containers are supported for Windows OS and multiple containers are supported for Linux OS.


We'd tend to phrase this as:

A `container` block as defined below. ~> **Note:** if `os_type` is set to `Windows` currently only a single container block is supported

tombuildsstuff · 2017-09-14T07:40:52Z

website/docs/r/container_group.html.markdown

+
+The `container` block supports:
+
+* `name` - (Required) Specifies the name of the Container.


Can we suffix this with "Changing this value forces a new resource to be created"?

tombuildsstuff · 2017-09-14T07:40:55Z

website/docs/r/container_group.html.markdown

+
+* `name` - (Required) Specifies the name of the Container.
+
+* `image` - (Required) The container image name.


Can we suffix this with "Changing this value forces a new resource to be created"?

tombuildsstuff · 2017-09-14T07:40:57Z

website/docs/r/container_group.html.markdown

+
+* `image` - (Required) The container image name.
+
+* `cpu` - (Required) The required number of CPU cores of the containers.


Can we suffix this with "Changing this value forces a new resource to be created"?

tombuildsstuff · 2017-09-14T07:41:01Z

website/docs/r/container_group.html.markdown

+
+* `cpu` - (Required) The required number of CPU cores of the containers.
+
+* `memory` - (Required) The required memory of the containers in GB.


Can we suffix this with "Changing this value forces a new resource to be created"?

tombuildsstuff · 2017-09-14T07:41:07Z

website/docs/r/container_group.html.markdown

+
+* `memory` - (Required) The required memory of the containers in GB.
+
+* `port` - (Optional) A public port for the container.


Can we suffix this with "Changing this value forces a new resource to be created"?

tombuildsstuff

A few minor points - but this otherwise LGTM (I'm just running the tests now :)

tombuildsstuff · 2017-09-14T10:51:53Z

azurerm/resource_arm_container_group.go

+			"ip_address": {
+				Type:     schema.TypeString,
+				Computed: true,
+				ForceNew: true,


given this is Computed, it doesn't need to be ForceNew - so we can remove this

tombuildsstuff · 2017-09-14T10:53:10Z

azurerm/resource_arm_container_group.go

+	}
+
+	containerConfigs := flattenContainerGroupContainers(resp.Containers)
+	d.Set("container", containerConfigs)


minor given this is setting a complex object, could we update this too:

containerConfigs := flattenContainerGroupContainers(resp.Containers) if err = d.Set("container", containerConfigs) { return fmt.Errorf("Error setting `container`: %+v", err) }

tombuildsstuff · 2017-09-14T10:54:44Z

azurerm/resource_arm_container_group.go

+		containerConfig["name"] = *container.Name
+		containerConfig["image"] = *container.Image
+
+		resourceRequests := *(*container.Resources).Requests


could we add an ifcheck around this to avoid Swagger/API issues? i.e.

if resources := container.Resources; resources != nil { if resourceRequests := resources.Requests; resourceRequests != nil { # ... } }

The API will always return these values. Changing just in case.

tombuildsstuff · 2017-09-14T10:55:07Z

azurerm/resource_arm_container_group.go

+
+			if v, ok := data["protocol"]; ok {
+				protocol := v.(string)
+				containerGroupPort.Protocol = containerinstance.ContainerGroupNetworkProtocol(strings.ToUpper(protocol))


does this actually need to be strings.ToUpper?

It's needed as the ContainerGroupNetworkProtocol enum is composed of only all uppercase strings.

tombuildsstuff · 2017-09-14T11:36:00Z

Hey @abhijeetgaiha

Sorry, I've unintentionally closed this by force-pushing the changes we discussed (to add an update test).. :( I'll re-open this in a separate PT (which will keep your commits)

ghost · 2020-04-01T15:21:01Z

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!

tombuildsstuff added enhancement new-resource labels Sep 8, 2017

tombuildsstuff reviewed Sep 12, 2017

View reviewed changes

tombuildsstuff reviewed Sep 14, 2017

View reviewed changes

tombuildsstuff closed this Sep 14, 2017

tombuildsstuff force-pushed the master branch from 6ab2d97 to c357dcd Compare September 14, 2017 11:34

tombuildsstuff mentioned this pull request Sep 14, 2017

New Resource: azurerm_container_group #333

Merged

tombuildsstuff added a commit that referenced this pull request Sep 14, 2017

Updating to include #333 & #311

033ce41

ghost locked and limited conversation to collaborators Apr 1, 2020


		* `location` - (Required) Specifies the supported Azure location where the resource exists. Changing this forces a new resource to be created. Changing this forces a new resource to be created.

		* `ip_address_type` - (Required) 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.


		The following attributes are exported:

		* `ip_address` - The IP address allocated to the container group.


		* `os_type` - (Required) The OS for the container group. Allowed values are `linux` and `windows` Changing this forces a new resource to be created.

		* `container` - (Required) The definition of a container that is part of the group. Currently, only single containers are supported for Windows OS and multiple containers are supported for Linux OS.


		The `container` block supports:

		* `name` - (Required) Specifies the name of the Container.


		* `name` - (Required) Specifies the name of the Container.

		* `image` - (Required) The container image name.


		* `image` - (Required) The container image name.

		* `cpu` - (Required) The required number of CPU cores of the containers.


		* `cpu` - (Required) The required number of CPU cores of the containers.

		* `memory` - (Required) The required memory of the containers in GB.


		* `memory` - (Required) The required memory of the containers in GB.

		* `port` - (Optional) A public port for the container.

Implementation of Azure container groups #311

Implementation of Azure container groups #311

Conversation

abhijeetgaiha commented Sep 7, 2017

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment