-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
azurerm_container_registry
- deprecated classic
sku
and storage_account_id
#11165
azurerm_container_registry
- deprecated classic
sku
and storage_account_id
#11165
Conversation
…e_account_id` The `classic` sku is deprecated by service. The [document](https://docs.microsoft.com/en-us/azure/container-registry/container-registry-skus) even doesn't show it up. Meanwhile, the service reject this sku for (at least) `PUT` request, (at least) since 2017-10-01 (used by provider v2.0.0). Since `storage_account_id` is used together with `classic` sku, this proeprty is also useless. Furthermore, this property has been removed in [Swagger](Azure/azure-rest-api-specs#13001).
845f9f1
to
ee069da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @magodo - comments have been left inline
@@ -56,11 +56,6 @@ func dataSourceContainerRegistry() *schema.Resource { | |||
Computed: true, | |||
}, | |||
|
|||
"storage_account_id": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
someone might be using this in a config so can we just deprecate it for now?
@@ -96,10 +91,6 @@ func dataSourceContainerRegistryRead(d *schema.ResourceData, meta interface{}) e | |||
d.Set("sku", string(sku.Tier)) | |||
} | |||
|
|||
if account := resp.StorageAccount; account != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and it could be an old account so maybe we should leave this till 3.0 - should be migrated isn't a 100% yes they are all migrated
DiffSuppressFunc: suppress.CaseDifference, | ||
ValidateFunc: validation.StringInSlice([]string{ | ||
string(containerregistry.Classic), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is probably fine if puts where failing, but what if someone had an old one that never udpated?
@@ -89,12 +88,6 @@ func resourceContainerRegistry() *schema.Resource { | |||
Default: true, | |||
}, | |||
|
|||
"storage_account_id": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, i don't see any reason to remove it now instead of deprecating it and removing in 3.0
@katbyte I've label this as a breaking-change now, let's wait until 3.0. |
kk @magodo - going to close this and we can revisit/reopen when we start 3.0 work |
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! |
The
classic
sku is deprecated by service. Thedocument even doesn't show it up. Meanwhile, the service reject this sku for (at least)
PUT
request, (at least) since 2017-10-01 (used by provider v2.0.0).Since
storage_account_id
is used together withclassic
sku, this proeprty is also useless. Furthermore, this property has been removed in Swagger.Besides, confirmed with ACR team that all the classic instances should have been migrated today. So it should be safe to deprecate this sku.