resource/container_group: add registry credential #1509
Conversation
This reverts commit 0dd53df.
| Required: true, | ||
| ForceNew: true, | ||
| }, | ||
| "username": { |
There was a problem hiding this comment.
Could we get a blank line between these properties like the rest of the resource?
| "server": { | ||
| Type: schema.TypeString, | ||
| Required: true, | ||
| ForceNew: true, |
There was a problem hiding this comment.
Could we get some simple validation here (and on username/password)?
ValidateFunc: validation.NoZeroValuesWould suffice
| Volumes: containerGroupVolumes, | ||
| OsType: containerinstance.OperatingSystemTypes(OSType), | ||
| Volumes: containerGroupVolumes, | ||
| ImageRegistryCredentials: credentials, |
There was a problem hiding this comment.
very minor but we could just directly assign this here
ImageRegistryCredentials: expandContainerGroupImageRegistryCredentials(d)| @@ -286,6 +314,12 @@ func resourceArmContainerGroupRead(d *schema.ResourceData, meta interface{}) err | |||
| } | |||
| d.Set("restart_policy", string(resp.RestartPolicy)) | |||
|
|
|||
| imageRegistryCredentials := flattenContainerGroupImageRegistryCredentials(d, resp.ImageRegistryCredentials) | |||
| err = d.Set("image_registry_credential", imageRegistryCredentials) | |||
| if err != nil { | |||
There was a problem hiding this comment.
Also minor, this could be combined into one line:
if err = d.Set("image_registry_credential", imageRegistryCredentials); err != nil {|
|
||
| for _, irc := range *credentials { | ||
| ircConfig := make(map[string]interface{}) | ||
| ircConfig["server"] = *irc.Server |
There was a problem hiding this comment.
Could we wrap these assignments in nil checks?
if v := ircConfig["server"] = *irc.Server; v != nil {
ircConfig["server"] = *v
}| ircConfig["server"] = *irc.Server | ||
| ircConfig["username"] = *irc.Username | ||
| // search for password in the old configs | ||
| for _, oldIrcConfig := range ircConfigsOld { |
There was a problem hiding this comment.
I'm not entirely sure, but i think the index will always be the same (otherwise each plan/apply we would attempt to update) so you might beable to simplify this to:
for i, oldIrcConfig := range ircConfigsOld {
...
if v, ok := d.GetOk(fmt.Sprintf("image_registry_credential.%d.password", i)); ok {
block["password"] = v.(string)
}
}
|
I was unable to write a test for the feature and had to revert it. It's functionally complete but lacks the test. The test requires setting up a private docker registry which I wasn't able to figure out in time. |
|
@tharvik I would recommend adding the test. |
* Added imageRegistryCredentials to container group * Update docs to add image_registry_credentials * Added test and fixes from pr #1509 * Fix up review feedback * Further tests and tweaks * Reverting the LinuxBasic test Tests pass: ``` $ acctests azurerm TestAccAzureRMContainerGroup_linuxBasic === RUN TestAccAzureRMContainerGroup_linuxBasic --- PASS: TestAccAzureRMContainerGroup_linuxBasic (86.67s) === RUN TestAccAzureRMContainerGroup_linuxBasicUpdate --- PASS: TestAccAzureRMContainerGroup_linuxBasicUpdate (107.92s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 194.641s ```
|
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
Found a serie of commits adding the registry credentials to a container group, made by @abhijeetgaiha, adding this feature. Reverting 0dd53df and c658442 makes it work.
Maybe @abhijeetgaiha wants to comments why he removed it two days after adding it, it's not really explained in the commits messages