azurerm_storage_share - Fix a bug that using storage_account_id accidentally leaks to the wrong branch for creation
#28063
Conversation
…ccidentally leaks to the wrong branch for creation
There was a problem hiding this comment.
Hi @magodo - Thanks for this PR. Unfortunately it breaks the intended purpose of having introduced the storage_account_id property, which is to discontinue use of the Data Plane API entirely when it is specified, as this is required for users that have no Terraform access to Data Plane endpoints for various reasons. Can you restore those code paths and take another look at resolving the referenced bug leaving that functionality intact?
Thanks
| @@ -160,6 +160,7 @@ func resourceStorageShare() *pluginsdk.Resource { | |||
| r.Schema["storage_account_name"] = &pluginsdk.Schema{ | |||
| Type: pluginsdk.TypeString, | |||
| Optional: true, | |||
| Computed: true, | |||
There was a problem hiding this comment.
We should not be setting this value if storage_account_id is set
| Computed: true, |
| @@ -171,6 +172,7 @@ func resourceStorageShare() *pluginsdk.Resource { | |||
| r.Schema["storage_account_id"] = &pluginsdk.Schema{ | |||
| Type: pluginsdk.TypeString, | |||
| Optional: true, | |||
| Computed: true, | |||
There was a problem hiding this comment.
We should not be setting this value if storage_account_id is set.
| Computed: true, |
| if accountName := d.Get("storage_account_name").(string); accountName != "" { | ||
| shareName := d.Get("name").(string) | ||
| quota := d.Get("quota").(int) | ||
| metaDataRaw := d.Get("metadata").(map[string]interface{}) | ||
| metaData := ExpandMetaData(metaDataRaw) |
There was a problem hiding this comment.
Changing this means that we will use the Data Plane client even in the cases that storage_account_id is specified in place of storage_account_name, which we specifically need to avoid for this change to separate the the two APIs. If storage_account_id is specified, we must use exclusively the Resource Manager API.
| if !features.FivePointOhBeta() && !strings.HasPrefix(d.Id(), "/subscriptions/") { | ||
| if !features.FivePointOhBeta() { |
There was a problem hiding this comment.
As above, this must be evaluated to ensure that cases where the ID is the resource manager ID, we exclusively use the Resource Manager API.
github-actions
bot
added
waiting-response
size/M
and removed
waiting-response
size/L
labels
magodo
force-pushed
the
storage_share_fix_bug_after_intro_sa_id
branch
from
e4b9826 to
4db7855
Compare
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Community Note
Description
With the introduction of
storage_account_idon theazurerm_storage_sharein #27733, there is one bug in the creation code, that if users specify the newstorage_account_idproperty (withstorage_account_nameomitted, as they are in conflict), the logic leaks to the v5 part, which is causing issues like #28032.This PR fixes that part by:
storage_account_namecheck, but infer the account name from the id ifstorage_account_idis usedstorage_account_[id|name]as Computed, per the breaking change guideline&& !strings.HasPrefix(d.Id(), "/subscriptions/")Additionally, I've reverted the changes on the acctest config, for the acl timestamp format, for the compatibility purpose. Note that the format is different between data plane and mgmt plane APIs. Meanwhile, I've modified the
flattenStorageShareACLsto convert the timestamp format back to the data plane version.PR Checklist
For example: “
resource_name_here- description of change e.g. adding propertynew_property_name_here”Changes to existing Resource / Data Source
Testing
Without v5 toggle
With v5 toggle
$ ARM_FIVEPOINTZERO_BETA=true TF_ACC=1 go test -v -timeout=20h -run=TestAccStorageShare_ ./internal/services/storage === RUN TestAccStorageShare_basicDeprecated storage_share_resource_test.go:26: skipping as not valid in 5.0 --- SKIP: TestAccStorageShare_basicDeprecated (0.00s) === RUN TestAccStorageShare_basic === PAUSE TestAccStorageShare_basic === RUN TestAccStorageShare_complete === PAUSE TestAccStorageShare_complete === RUN TestAccStorageShare_requiresImportDeprecated storage_share_resource_test.go:75: skipping as not valid in 5.0 --- SKIP: TestAccStorageShare_requiresImportDeprecated (0.00s) === RUN TestAccStorageShare_requiresImport === PAUSE TestAccStorageShare_requiresImport === RUN TestAccStorageShare_disappearsDeprecated storage_share_resource_test.go:109: skipping as not valid in 5.0 --- SKIP: TestAccStorageShare_disappearsDeprecated (0.00s) === RUN TestAccStorageShare_deleteAndRecreateDeprecated storage_share_resource_test.go:124: skipping as not valid in 5.0 --- SKIP: TestAccStorageShare_deleteAndRecreateDeprecated (0.00s) === RUN TestAccStorageShare_metaDataDeprecated storage_share_resource_test.go:153: skipping as not valid in 5.0 --- SKIP: TestAccStorageShare_metaDataDeprecated (0.00s) === RUN TestAccStorageShare_metaData === PAUSE TestAccStorageShare_metaData === RUN TestAccStorageShare_aclDeprecated storage_share_resource_test.go:201: skipping as not valid in 5.0 --- SKIP: TestAccStorageShare_aclDeprecated (0.00s) === RUN TestAccStorageShare_acl === PAUSE TestAccStorageShare_acl === RUN TestAccStorageShare_aclGhostedRecallDeprecated storage_share_resource_test.go:263: skipping as not valid in 5.0 --- SKIP: TestAccStorageShare_aclGhostedRecallDeprecated (0.00s) === RUN TestAccStorageShare_aclGhostedRecall === PAUSE TestAccStorageShare_aclGhostedRecall === RUN TestAccStorageShare_updateQuotaDeprecated storage_share_resource_test.go:297: skipping as not valid in 5.0 --- SKIP: TestAccStorageShare_updateQuotaDeprecated (0.00s) === RUN TestAccStorageShare_updateQuota === PAUSE TestAccStorageShare_updateQuota === RUN TestAccStorageShare_largeQuotaDeprecated storage_share_resource_test.go:343: skipping as not valid in 5.0 --- SKIP: TestAccStorageShare_largeQuotaDeprecated (0.00s) === RUN TestAccStorageShare_largeQuota === PAUSE TestAccStorageShare_largeQuota === RUN TestAccStorageShare_accessTierStandardDeprecated storage_share_resource_test.go:391: skipping as not valid in 5.0 --- SKIP: TestAccStorageShare_accessTierStandardDeprecated (0.00s) === RUN TestAccStorageShare_accessTierStandard === PAUSE TestAccStorageShare_accessTierStandard === RUN TestAccStorageShare_accessTierPremiumDeprecated storage_share_resource_test.go:439: skipping as not valid in 5.0 --- SKIP: TestAccStorageShare_accessTierPremiumDeprecated (0.00s) === RUN TestAccStorageShare_accessTierPremium === PAUSE TestAccStorageShare_accessTierPremium === RUN TestAccStorageShare_nfsProtocolDeprecated storage_share_resource_test.go:473: skipping as not valid in 5.0 --- SKIP: TestAccStorageShare_nfsProtocolDeprecated (0.00s) === RUN TestAccStorageShare_nfsProtocol === PAUSE TestAccStorageShare_nfsProtocol === RUN TestAccStorageShare_protocolUpdateDeprecated storage_share_resource_test.go:508: skipping as not valid in 5.0 --- SKIP: TestAccStorageShare_protocolUpdateDeprecated (0.00s) === RUN TestAccStorageShare_protocolUpdate === PAUSE TestAccStorageShare_protocolUpdate === CONT TestAccStorageShare_accessTierStandard === CONT TestAccStorageShare_basic === CONT TestAccStorageShare_largeQuota === CONT TestAccStorageShare_nfsProtocol === CONT TestAccStorageShare_accessTierPremium === CONT TestAccStorageShare_metaData === CONT TestAccStorageShare_aclGhostedRecall === CONT TestAccStorageShare_requiresImport === CONT TestAccStorageShare_complete === CONT TestAccStorageShare_protocolUpdate === CONT TestAccStorageShare_acl === CONT TestAccStorageShare_updateQuota --- PASS: TestAccStorageShare_requiresImport (104.31s) --- PASS: TestAccStorageShare_basic (105.14s) --- PASS: TestAccStorageShare_aclGhostedRecall (105.25s) --- PASS: TestAccStorageShare_nfsProtocol (105.77s) --- PASS: TestAccStorageShare_accessTierPremium (105.82s) --- PASS: TestAccStorageShare_complete (106.34s) --- PASS: TestAccStorageShare_updateQuota (109.27s) --- PASS: TestAccStorageShare_metaData (130.78s) --- PASS: TestAccStorageShare_accessTierStandard (133.63s) --- PASS: TestAccStorageShare_largeQuota (134.36s) --- PASS: TestAccStorageShare_protocolUpdate (233.51s) --- PASS: TestAccStorageShare_acl (260.46s) PASS ok github.com/hashicorp/terraform-provider-azurerm/internal/services/storage 260.500sChange Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_storage_share- Fix a bug that usingstorage_account_idaccidentally leaks to the wrong branch for creation [GH-00000]This is a (please select all that apply):
Related Issue(s)
Fixes #28032
Note
If this PR changes meaningfully during the course of review please update the title and description as required.