-
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
[Enhance] -azurerm_machine_learning_datastore_datalake_gen2
- Support crosse sub storage account
#28123
Conversation
39d1836
to
5ff3849
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 for this @xuzhang3 - I have just left one comment to note that we currently have an env var for an alternative subscription id. Could we update the tests to use this instead and re-run them and then we can take another look at this? Thanks!
if os.Getenv("ARM_TEST_ACC_DATASTORE_GEN2_CROSS_SUB_SA_CONTAINER") == "" { | ||
t.Skip("ARM_TEST_ACC_DATASTORE_GEN2_CROSS_SUB_SA_CONTAINER not set") | ||
} |
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.
Could we remove this and use ARM_SUBSCRIPTION_ID_ALT
instead for an alternative subscription 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.
@catriona-m test case updated.
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 for updating this @xuzhang3, I've just left two minor suggestions regarding resource names for the tests, but otherwise this is looking good. Thanks!
internal/services/machinelearning/machine_learning_datastore_datalake_gen2_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/machinelearning/machine_learning_datastore_datalake_gen2_resource_test.go
Outdated
Show resolved
Hide resolved
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 @xuzhang3 LGTM!
Community Note
Description
PR Checklist
For example: “
resource_name_here
- description of change e.g. adding propertynew_property_name_here
”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_resource
- support for thething1
property [GH-00000]This is a (please select all that apply):
Related Issue(s)
Fixes #27254
Relate PR: #27256
This PR will add the subscription ID and resource group to the service so we know where the storage account located, also works for cross subscription. For the existing resource managed by AzureRM the subscription ID and resource group are null, AzureRM cannot find the storage account if it's not in the same sub as MLW, this PR will fix this.
Note
If this PR changes meaningfully during the course of review please update the title and description as required.