provider/azurerm: Container Registry #10973

tombuildsstuff · 2016-12-30T18:30:09Z

The access_key for the Storage Account isn't returned from the server once the Container Registry has been created (for obvious reasons) - so I've ignored that object in the import tests for now.

tombuildsstuff · 2016-12-30T18:30:17Z

Tests pass

$ TF_ACC=1 go test ./builtin/providers/azurerm -v -run TestAccAzureRMContainerRegistry -timeout 120m
=== RUN   TestAccAzureRMContainerRegistry_importBasic
--- PASS: TestAccAzureRMContainerRegistry_importBasic (123.07s)
=== RUN   TestAccAzureRMContainerRegistry_importComplete
--- PASS: TestAccAzureRMContainerRegistry_importComplete (124.61s)
=== RUN   TestAccAzureRMContainerRegistryName_validation
--- PASS: TestAccAzureRMContainerRegistryName_validation (0.00s)
=== RUN   TestAccAzureRMContainerRegistry_basic
--- PASS: TestAccAzureRMContainerRegistry_basic (123.95s)
=== RUN   TestAccAzureRMContainerRegistry_complete
--- PASS: TestAccAzureRMContainerRegistry_complete (121.54s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/azurerm	493.199s

stack72

Hey @tombuildsstuff

This is looking good - few questions left inline

Thanks

Paul

stack72 · 2016-12-30T23:48:32Z

builtin/providers/azurerm/import_arm_container_registry_test.go

+				ResourceName:            resourceName,
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"storage_account"},


Why do we ignore the storage_account?

This is because the inner access_key field isn't returned through the API after the initial CreateOrUpdate. Potentially we could retrieve it via the Storage Account - but it felt like the wrong thing to do (given it's a completely unrelated object)?

ok, that makes sense

stack72 · 2016-12-30T23:49:44Z

builtin/providers/azurerm/resource_arm_container_registry.go

+		Tags: expandTags(tags),
+	}
+
+	if v, ok := d.GetOk("storage_account"); ok {


If this is a required field, we don't need the if v, ok part - we can assume it's always there

stack72 · 2016-12-30T23:50:02Z

builtin/providers/azurerm/resource_arm_container_registry.go

+		storageAccountName := account["name"].(string)
+		storageAccountAccessKey := account["access_key"].(string)
+		parameters.RegistryProperties.StorageAccount = &containerregistry.StorageAccountProperties{
+			Name:      &storageAccountName,


I think this should be wrapped in azure.String()

stack72 · 2016-12-30T23:50:30Z

builtin/providers/azurerm/resource_arm_container_registry.go

+		}
+	}
+
+	_, err := client.CreateOrUpdate(resourceGroup, name, parameters)


Does this handle the polling or do we need to do that in our code using a staterefreshfunc?

So, the SDK says that it's only possible to get a 200 OK back (i.e. that it's not a long running operation) - which I've confirmed in all of the testing I've done with this, where the Registry's created so quickly I've not seen it block.

The Documentation on the other hand specifies that it's possible to get a 202 Accepted back - however I've noticed a couple of issues in the Documentation anyway so this may well be a bug in the docs.

Given they're both generated from the same place I'll file an issue - but as yet I've not seen a 202 in real world usage.

Opened an issue: Azure/azure-rest-api-specs#842

tombuildsstuff · 2017-01-02T11:08:45Z

Updated and re-ran the tests:

$ TF_ACC=1 go test ./builtin/providers/azurerm -v -run TestAccAzureRMContainerRegi -timeout 120m
=== RUN   TestAccAzureRMContainerRegistry_importBasic
--- PASS: TestAccAzureRMContainerRegistry_importBasic (131.18s)
=== RUN   TestAccAzureRMContainerRegistry_importComplete
--- PASS: TestAccAzureRMContainerRegistry_importComplete (127.82s)
=== RUN   TestAccAzureRMContainerRegistryName_validation
--- PASS: TestAccAzureRMContainerRegistryName_validation (0.00s)
=== RUN   TestAccAzureRMContainerRegistry_basic
--- PASS: TestAccAzureRMContainerRegistry_basic (119.65s)
=== RUN   TestAccAzureRMContainerRegistry_complete
--- PASS: TestAccAzureRMContainerRegistry_complete (125.95s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/azurerm	504.617s

stack72 · 2017-01-02T14:37:42Z

This LGTM! thanks for the changes :)

% make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccAzureRMContainerRegi'                                                                                                           2 ↵
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/01/02 14:28:13 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/azurerm -v -run=TestAccAzureRMContainerRegi -timeout 120m
=== RUN   TestAccAzureRMContainerRegistry_importBasic
--- PASS: TestAccAzureRMContainerRegistry_importBasic (131.46s)
=== RUN   TestAccAzureRMContainerRegistry_importComplete
--- PASS: TestAccAzureRMContainerRegistry_importComplete (131.55s)
=== RUN   TestAccAzureRMContainerRegistryName_validation
--- PASS: TestAccAzureRMContainerRegistryName_validation (0.00s)
=== RUN   TestAccAzureRMContainerRegistry_basic
--- PASS: TestAccAzureRMContainerRegistry_basic (125.96s)
=== RUN   TestAccAzureRMContainerRegistry_complete
--- PASS: TestAccAzureRMContainerRegistry_complete (128.36s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/azurerm	517.339s

ghost · 2020-04-18T02:10:49Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

tombuildsstuff added 9 commits December 30, 2016 15:58

Fixing the indentation

68e9e26

Adding the Container Registry SDK

9030309

Implementing the container registry

eb47ac7

Enabling the provider / registering the Resource Provider

92755cc

Acceptance Tests

de815b7

Documentation for Container Registry

1faaa33

Fixing the name validation

eafd995

Validation for the Container Registry Name

2981411

Added Import support for Containr Registry

4d8c1be

stack72 reviewed Dec 30, 2016

View reviewed changes

Storage Account is no longer optional

cb26e61

tombuildsstuff added 2 commits January 2, 2017 11:10

Updating the docs

ccfabc4

Forcing a re-run in Travis

6e24ac3

stack72 added enhancement new-resource provider/azurerm labels Jan 2, 2017

stack72 merged commit 874ec8b into hashicorp:master Jan 2, 2017

ghost locked and limited conversation to collaborators Apr 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/azurerm: Container Registry #10973

provider/azurerm: Container Registry #10973

tombuildsstuff commented Dec 30, 2016

tombuildsstuff commented Dec 30, 2016

stack72 left a comment

stack72 Dec 30, 2016

tombuildsstuff Dec 31, 2016

stack72 Jan 2, 2017

stack72 Dec 30, 2016

tombuildsstuff Jan 2, 2017

stack72 Dec 30, 2016

tombuildsstuff Jan 2, 2017

stack72 Dec 30, 2016

tombuildsstuff Dec 31, 2016

tombuildsstuff Jan 2, 2017

tombuildsstuff commented Jan 2, 2017

stack72 commented Jan 2, 2017

ghost commented Apr 18, 2020

provider/azurerm: Container Registry #10973

provider/azurerm: Container Registry #10973

Conversation

tombuildsstuff commented Dec 30, 2016

tombuildsstuff commented Dec 30, 2016

stack72 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

tombuildsstuff commented Jan 2, 2017

stack72 commented Jan 2, 2017

ghost commented Apr 18, 2020