resource/container_group: add registry credential #1529

lawrencegripper · 2018-07-10T13:10:10Z

When I noticed this feature was missing I checked the issues and didn't see anything so set about working on it. When I came to raise the PR I noticed that #1509 was already in progress.

As I'd already included docs, test and an example (not in the current PR work) I thought it's still worth raising this PR as these can be merged into the existing PR and don't go to waste.

cc/ @tharvik and @katbyte

Ps. will check open PR's next time :(

katbyte · 2018-07-10T21:33:05Z

Hi @lawrencegripper,

Regardless thank you for your contribution! As #1509 was just pulling some old commits out of the repo and lacks documentation/tests I am going to close it in favour of yours.

I will review it shortly.

katbyte

@lawrencegripper,

I've left a few inline comments but otherwise this mostly LGTM! Look forward to getting this merged once they are addressed.

katbyte · 2018-07-10T23:49:00Z

azurerm/resource_arm_container_group.go

@@ -277,6 +309,10 @@ func resourceArmContainerGroupRead(d *schema.ResourceData, meta interface{}) err
 	}
 	flattenAndSetTags(d, resp.Tags)

+	if imageRegCreds := flattenContainerImageRegistryCredentials(d, resp.ImageRegistryCredentials); imageRegCreds != nil {
+		d.Set("image_registry_credential", imageRegCreds)


Could we check for an error here? like:

if err := d.Set("capabilities", flattenAzureRmCosmosDBAccountCapabilities(resp.Capabilities)); err != nil { return fmt.Errorf("Error setting `capabilities`: %+v", err) }

d.Set() sill handle a nil value correctly

Sure, make sense I'll fix this up

katbyte · 2018-07-10T23:58:26Z

azurerm/resource_arm_container_group.go

+		password := credConfig["password"].(string)
+
+		output = append(output, containerinstance.ImageRegistryCredential{
+			Server:   &server,


Very minor but these could be assigned inline via utils.String()

Server: utils.String(credConfig["username"].(string)),

Ah much nicer, I thought about bringing in the to package from Autorest as has lots of these funcs too didn't realize you already had one.

katbyte · 2018-07-11T00:00:28Z

azurerm/resource_arm_container_group.go

+			credConfig["username"] = *cred.Username
+		}
+
+		for i, configsOld := range configsOld {


I'm not sure, but i presume the ordering is consistent so you could get the old values this way:

for i, oldIrcConfig := range ircConfigsOld { ... if v, ok := d.GetOk(fmt.Sprintf("image_registry_credential.%d.password", i)); ok { block["password"] = v.(string) } }

I'm a little uncertain on this one so was being careful. Does the ordering of elements relate to the ordering of the elements in HCL? So for example if the user update the password element in their template but also moved the credential block ahead of another existing block would that affect this approach (making sure the server name matches)? Feels like an edge case either way round.

Yes, the order of the list directly relates to the order of the elements in HCL. So if the user does as you describe, the Apply would be sending a new list anyways, and the subsequent read would be post Update. unless azure returns things in a different order I think it should be fine. You could always write a test to confirm 🙂

Cool, makes sense to me. Will also check with the test just to be on the safe side.

katbyte · 2018-07-11T00:00:56Z

azurerm/resource_arm_container_group_test.go

+    image  = "microsoft/aci-helloworld:latest"
+    cpu    = "0.5"
+    memory = "0.5"
+	port   = "80"


could we fix the alignment here?

Woop will do

lawrencegripper · 2018-07-13T15:59:48Z

Tests look good and hopefully all feedback is sorted.

2018/07/13 16:56:10 debugger.go:518: continuing
=== RUN   TestAccAzureRMContainerGroup_imageRegistryCredentials
--- PASS: TestAccAzureRMContainerGroup_imageRegistryCredentials (62.86s)
=== RUN   TestAccAzureRMContainerGroup_imageRegistryCredentialsUpdate
--- PASS: TestAccAzureRMContainerGroup_imageRegistryCredentialsUpdate (75.83s)
PASS

tombuildsstuff · 2018-07-16T10:42:31Z

azurerm/resource_arm_container_group_test.go

 func TestAccAzureRMContainerGroup_linuxBasic(t *testing.T) {
 	resourceName := "azurerm_container_group.test"
 	ri := acctest.RandInt()

-	config := testAccAzureRMContainerGroup_linuxBasic(ri, testLocation())
+	config := testAccAzureRMContainerGroup_imageRegistryCredentials(ri, testLocation())


the tests are failing since this has been updated but the counts are incorrect; going to push a test to revert this to TestAccAzureRMContainerGroup_linuxBasic

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 ```

tombuildsstuff

LGTM - thanks for this 👍

changes have been pushed

tombuildsstuff · 2018-07-16T11:04:14Z

Tests pass with the test fix:

lawrencegripper · 2018-07-16T12:28:27Z

Thanks @tombuildsstuff, sorry for messing up the linuxBasic tests was only running the new tests I'd added so I missed it - will remember to run all tests in future.

tombuildsstuff · 2018-07-16T12:29:33Z

@lawrencegripper no worries - easily done, thanks again for this PR :)

ghost · 2020-03-30T15:18:44Z

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!

lawrencegripper added 3 commits July 10, 2018 13:26

Added imageRegistryCredentials to container group

6f133de

Update docs to add image_registry_credentials

6434cf6

Merge remote-tracking branch 'upstream/master' into lg/addcredstoaci

73b236a

lawrencegripper force-pushed the lg/addcredstoaci branch from dd3f7dd to eb822f3 Compare July 10, 2018 13:11

Added test and fixes from pr hashicorp#1509

af8f84b

lawrencegripper force-pushed the lg/addcredstoaci branch from eb822f3 to af8f84b Compare July 10, 2018 13:13

katbyte mentioned this pull request Jul 10, 2018

resource/container_group: add registry credential #1509

Closed

katbyte previously requested changes Jul 11, 2018

View reviewed changes

katbyte changed the title ~~resource/container_group: add registry credential v2 (oops)~~ resource/container_group: add registry credential Jul 11, 2018

Fix up review feedback

06aa604

lawrencegripper force-pushed the lg/addcredstoaci branch from 087eaff to 06aa604 Compare July 11, 2018 10:47

katbyte added this to the 1.10.0 milestone Jul 11, 2018

katbyte added service/container-service enhancement labels Jul 11, 2018

Further tests and tweaks

1114183

tombuildsstuff reviewed Jul 16, 2018

View reviewed changes

tombuildsstuff approved these changes Jul 16, 2018

View reviewed changes

tombuildsstuff merged commit fe9975d into hashicorp:master Jul 16, 2018

tombuildsstuff added a commit that referenced this pull request Jul 16, 2018

Updating to include #1529

a0b3434

lawrencegripper mentioned this pull request Sep 4, 2018

Feature Request: Support Image Registry Credentials for Azure Container Instance Groups #1142

Closed

tombuildsstuff mentioned this pull request Nov 2, 2018

Add support for ACI with public container registries #287

Closed

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource/container_group: add registry credential #1529

resource/container_group: add registry credential #1529

lawrencegripper commented Jul 10, 2018

katbyte commented Jul 10, 2018

katbyte left a comment

katbyte Jul 10, 2018

lawrencegripper Jul 11, 2018

katbyte Jul 10, 2018

lawrencegripper Jul 11, 2018

katbyte Jul 11, 2018

lawrencegripper Jul 11, 2018 •

edited

Loading

katbyte Jul 11, 2018

lawrencegripper Jul 13, 2018

katbyte Jul 11, 2018

lawrencegripper Jul 11, 2018

lawrencegripper commented Jul 13, 2018 •

edited

Loading

tombuildsstuff Jul 16, 2018

tombuildsstuff left a comment

tombuildsstuff commented Jul 16, 2018

lawrencegripper commented Jul 16, 2018

tombuildsstuff commented Jul 16, 2018

ghost commented Mar 30, 2020

resource/container_group: add registry credential #1529

resource/container_group: add registry credential #1529

Conversation

lawrencegripper commented Jul 10, 2018

katbyte commented Jul 10, 2018

katbyte 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

lawrencegripper Jul 11, 2018 • edited Loading

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

lawrencegripper commented Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Jul 16, 2018

lawrencegripper commented Jul 16, 2018

tombuildsstuff commented Jul 16, 2018

ghost commented Mar 30, 2020

lawrencegripper Jul 11, 2018 •

edited

Loading

lawrencegripper commented Jul 13, 2018 •

edited

Loading