Container groups: Add support for env vars and command line #336
Conversation
There was a problem hiding this comment.
Hey @abhijeetgaiha
Thanks for this PR :)
I've taken a look through and left some comments in-line, but this generally looks good.
Thanks!
| @@ -197,6 +188,9 @@ func resourceArmContainerGroupRead(d *schema.ResourceData, meta interface{}) err | |||
| resp, err := containterGroupsClient.Get(resGroup, name) | |||
|
|
|||
| if err != nil { | |||
| if utils.ResponseWasNotFound(resp.Response) { | |||
| return nil | |||
There was a problem hiding this comment.
given the Read method is used for several things (both checking that a resource exists at Plan time, and for parsing data from the API) - can we add d.SetId("") before the return nil here to ensure that deleted resources are detected?
| @@ -265,12 +259,33 @@ func flattenContainerGroupContainers(containers *[]containerinstance.Container) | |||
| } | |||
| // protocol isn't returned in container config | |||
|
|
|||
| if len(*container.EnvironmentVariables) > 0 { | |||
There was a problem hiding this comment.
this'll crash if container.EnvironmentVariables is nil - could we add an if statement around this?
There was a problem hiding this comment.
It's never nil, but a check is warranted to be safe. Done.
There was a problem hiding this comment.
To add some context to why this check is needed: if the Swagger is incorrect (which historically has happened a lot) this would be mapped incorrectly and crash
| resource.TestCheckResourceAttr(resourceName, "ip_address_type", "Public"), | ||
| resource.TestCheckResourceAttr(resourceName, "os_type", "Linux"), | ||
| resource.TestCheckResourceAttr(resourceName, "tags.%", "1"), | ||
| resource.TestCheckResourceAttr(resourceName, "tags.environment", "testing"), |
There was a problem hiding this comment.
I'd suggest that checking all of these is probably overkill, since the Import tests will do this for us (by deploying the input configuration, and then comparing the remote state against it - without specifying each fields values)
Instead - we could probably just check the OS Type / Environment Variables here?
There was a problem hiding this comment.
In addition, I'll leave command in there too, remove the others.
| @@ -32,6 +32,10 @@ resource "azurerm_container_group" "aci-helloworld" { | |||
| cpu ="0.5" | |||
| memory = "1.5" | |||
| port = "80" | |||
| env_vars { | |||
| "env"="staging" | |||
There was a problem hiding this comment.
it might sound odd, but in this current example the Environment Variable looks very similar to Tags (which we normally use "Environment" = "Staging" as an example) - as such would it make more sense to make this "NODE_ENV" = "Staging"?
There was a problem hiding this comment.
I made them the same intentionally, but this change is fine.
| @@ -108,25 +108,16 @@ func resourceArmContainerGroup() *schema.Resource { | |||
| }, true), | |||
| }, | |||
|
|
|||
| "env_var": { | |||
| Type: schema.TypeList, | |||
| "env_vars": { | |||
There was a problem hiding this comment.
is it worth making this environment_variables?
There was a problem hiding this comment.
Done. Also matches with the CLI.
| Type: schema.TypeList, | ||
| "env_vars": { | ||
| Type: schema.TypeMap, | ||
| Optional: true, |
There was a problem hiding this comment.
be aware, if we'd done a release of this, we'd have to include a State Migration to handle converting between the two fields going from a TypeList -> TypeMap - but I think we can probably overlook that this time since this hasn't been released yet
tombuildsstuff
force-pushed
the
master
branch
from
d17f014 to
59d9025
Compare
|
@abhijeetgaiha sorry, messed up git again (I think it's pushing to a remote |
|
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
More progress on #287