Skip to content
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

[New Resource] - azurerm_machine_learning_datastore #19867

Closed
wants to merge 3 commits into from

Conversation

xuzhang3
Copy link
Contributor

@xuzhang3 xuzhang3 commented Jan 5, 2023

Fixes: #14701

=== CONT  TestAccMachineLearningDataStore_dataLakeGen2WithSpn
=== CONT  TestAccMachineLearningDataStore_dataLakeGen2Basic
--- PASS: TestAccMachineLearningDataStore_dataLakeGen2Basic (636.77s)
--- PASS: TestAccMachineLearningDataStore_blobStorageAccountKey (638.04s)
--- PASS: TestAccMachineLearningDataStore_blobStorageSasToken (643.68s)
--- PASS: TestAccMachineLearningDataStore_dataLakeGen2WithSpn (643.91s)
--- PASS: TestAccMachineLearningDataStore_fileShareSas (651.29s)
--- PASS: TestAccMachineLearningDataStore_fileShareAccountKey (663.93s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/machinelearning       682.103s

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR @xuzhang3, this will be a great addition to machine learning services! There are however a few larger changes that need to happen before we can consider introducing this resource.

  1. Since this resource can support multiple types, we should actually split this out into a resource for each type - the user experience will be better and more consistent.
  2. The resources need to be typed, since they will need to be typed in the future for resource generation from Pandora.

Comment on lines +48 to +50
"resource_group_name": commonschema.ResourceGroupName(),

"workspace_name": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can reduce these two properties down to one workspace_id

ValidateFunc: validate.WorkspaceName,
},

"type": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't expose type, this resource should probably be split out into a separate resource for each type. In addition Data Lake Gen 1 is EOL, so perhaps we should only consider creating a resource for this if explicitly requested?

@xuzhang3
Copy link
Contributor Author

xuzhang3 commented Jan 6, 2023

@stephybun thanks for reviewing this PR, I will close this PR and submit another PR for different types of this resources

@github-actions
Copy link

github-actions bot commented Feb 6, 2023

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Azure Machine Learning datastores
2 participants