New Data Source: azurerm_container_registry
#1642
Conversation
There was a problem hiding this comment.
hey @schoren
Thanks for this PR :)
I've taken a look through and left some (mostly minor) comments in-line but this is looking good; the failing tests you're referring to in #1634 are because the fields are missing from the schema - if we add them to the schema (I've added an example in the review) then these should pass and we should be able to run the tests / get this merged 👍
Thanks!
| State: schema.ImportStatePassthrough, | ||
| }, | ||
| MigrateState: resourceAzureRMContainerRegistryMigrateState, | ||
| SchemaVersion: 2, |
There was a problem hiding this comment.
we should be able to remove the Importer -> Schema version lines here, since they're not needed for data sources
| page_title: "Azure Resource Manager: azurerm_container_registry" | ||
| sidebar_current: "docs-azurerm-datasource-image" | ||
| description: |- | ||
| Get information about an Image |
There was a problem hiding this comment.
can we make this Use this data source to access information about a Container Registry. to match the comment below?
| --- | ||
| layout: "azurerm" | ||
| page_title: "Azure Resource Manager: azurerm_container_registry" | ||
| sidebar_current: "docs-azurerm-datasource-image" |
There was a problem hiding this comment.
can we make this "docs-azurerm-datasource-container-registry"?
|
|
||
| output "image_id" { | ||
| value = "${data.azurerm_container_registry.test.id}" | ||
| } |
There was a problem hiding this comment.
minor could we make this login_server which would be a more likely output here? e.g.
output "login_server" {
value = "${data.azurerm_container_registry.test.login_server}"
}
|
|
||
| * `admin_username` - The Username associated with the Container Registry Admin account - if the admin account is enabled. | ||
|
|
||
| * `admin_password` - The Password associated with the Container Registry Admin account - if the admin account is enabled. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ValidateFunc: validateAzureRMContainerRegistryName, | ||
| }, | ||
|
|
||
| "resource_group_name": resourceGroupNameSchema(), |
There was a problem hiding this comment.
can we swap resourceGroupNameSchema for resourceGroupNameForDataSourceSchema here?
| }, | ||
|
|
||
| "resource_group_name": resourceGroupNameSchema(), | ||
| }, |
There was a problem hiding this comment.
there's other fields being set below which don't exist in the Schema - can we add them? They'll be computed only fields since this is a Data Source, so this'll be:
"location": locationForDataSourceSchema(),
"admin_enabled": {
Type: schema.TypeBool,
Computed: true,
},
"admin_password": {
Type: schema.TypeString,
Computed: true,
},
"admin_username": {
Type: schema.TypeString,
Computed: true,
},
"login_server": {
Type: schema.TypeString,
Computed: true,
},
"sku": {
Type: schema.TypeString,
Computed: true,
},
"storage_account_id": {
Type: schema.TypeString,
Computed: true,
},
| @@ -0,0 +1,42 @@ | |||
| --- | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
tombuildsstuff
changed the title
azurerm_container_registry
schoren
force-pushed
the
feature-datasource-acr
branch
from
596c7f6 to
b233cef
Compare
|
@tombuildsstuff Thanks for your comments and help. I've made the changes as suggested, and now the tests are passing. Let me know if I missed something and I'll fix it. |
There was a problem hiding this comment.
hey @schoren
Thanks for pushing those changes - taking one last look through I've noticed a couple of minor things which need to be resolved prior to merging; but this now otherwise LGTM 👍
Thanks!
| return &schema.Resource{ | ||
| Read: dataSourceArmContainerRegistryRead, | ||
| MigrateState: resourceAzureRMContainerRegistryMigrateState, | ||
| SchemaVersion: 2, |
There was a problem hiding this comment.
can we remove these two lines MigrateState / SchemaVersion - since they're not needed?
| if utils.ResponseWasNotFound(resp.Response) { | ||
| log.Printf("[DEBUG] Container Registry %q was not found in Resource Group %q", name, resourceGroup) | ||
| d.SetId("") | ||
| return nil |
There was a problem hiding this comment.
since this is a Data Source, if this isn't found we should return an error; thus can we make this:
return fmt.Errorf("Container Registry %q was not found in Resource Group %q", name, resourceGroup)
* Remove unused Schema fields * Return error if resource is not found
| ri := acctest.RandInt() | ||
| config := testAccDataSourceAzureRMContainerRegistry_basic(ri) | ||
|
|
||
| dataSourceName := "data.azurerm_container_registry.test" |
There was a problem hiding this comment.
re-reviewing this I noticed this was checking the resource (azurerm_container_registry.test) rather than the data source (which has the data. prefix) - I've pushed a commit to fix this (I hope you don't mind!)
There was a problem hiding this comment.
Of course not! Thanks for taking the time to review this and making this fix. Sorry for all this little erros
There was a problem hiding this comment.
not at all - apologies I missed it the first time around; thanks for this contribution, this'll go out in v1.12 :)
There was a problem hiding this comment.
hey @schoren
Thanks for pushing those changes - this now LGTM; I'll kick off the tests now 👍
Thanks!
|
Test passes:
|
|
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
This PR fixes #1634