New Resource: `azurerm_machine_learning_registry` #22870

wuxu92 · 2023-08-09T05:38:05Z

Draft for swagger API Issue

there is a cache inconsistency issue in the UPDATE, ~~so it may still read the old value after updating the tags field. but only the tags field can be updated. is this acceptable for this resource?~~

there are eventually consistency issues in Update/RemoveRegions API. This resource can be merged once the issue addressed by service team. I have reach out to service team but I am still waiting for their response.

--- PASS: TestAccMachineLearningRegistry_basic (795.53s)
--- PASS: TestAccMachineLearningRegistry_complete (843.55s)
--- PASS: TestAccMachineLearningRegistry_update (989.24s)

stephybun

Thanks @wuxu92. I've made a first pass and have some questions regarding the design of this resource. I think there's some work to be done there.

stephybun · 2023-08-09T07:39:01Z

internal/services/machinelearning/client/client.go

+	Datastore                *datastore.DatastoreClient
+	MachineLearningComputes  *machinelearningcomputes.MachineLearningComputesClient
+	Workspaces               *workspaces.WorkspacesClient
+	RegistryManagementClient *registrymanagement.RegistryManagementClient


Alphabetical ordering

Suggested change

Datastore *datastore.DatastoreClient

MachineLearningComputes *machinelearningcomputes.MachineLearningComputesClient

Workspaces *workspaces.WorkspacesClient

RegistryManagementClient *registrymanagement.RegistryManagementClient

Datastore *datastore.DatastoreClient

MachineLearningComputes *machinelearningcomputes.MachineLearningComputesClient

RegistryManagementClient *registrymanagement.RegistryManagementClient

Workspaces *workspaces.WorkspacesClient

stephybun · 2023-08-09T07:39:09Z

internal/services/machinelearning/client/client.go

+		Datastore:                datastoreClient,
+		MachineLearningComputes:  computesClient,
+		RegistryManagementClient: registryClient,
+		Workspaces:               workspacesClient,


stephybun · 2023-08-10T13:06:58Z

internal/services/machinelearning/machine_learning_registry_resource.go

+			// with this stateWait, it can still read old value if from different server cluster.
+			deadline, ok := ctx.Deadline()
+			if !ok {
+				return fmt.Errorf("no deadline for ctx")


Suggested change

return fmt.Errorf("no deadline for ctx")

return fmt.Errorf("internal-error: no deadline for ctx")

stephybun · 2023-08-10T13:12:02Z

internal/services/machinelearning/machine_learning_registry_resource.go

+		"region_details": {
+			Type:     pluginsdk.TypeList,
+			Required: true,
+			ForceNew: true,
+			MinItems: 1,
+			Elem: &pluginsdk.Resource{
+				Schema: map[string]*pluginsdk.Schema{
+					"acr_details": {
+						Type:     pluginsdk.TypeList,
+						Optional: true,
+						Elem: &pluginsdk.Resource{
+							Schema: map[string]*pluginsdk.Schema{
+								"system_created_acr_account": {
+									Type:     pluginsdk.TypeList,
+									Optional: true,
+									Elem: &pluginsdk.Resource{
+										Schema: map[string]*pluginsdk.Schema{
+											"acr_account_name": {
+												Type:     pluginsdk.TypeString,
+												Computed: true,
+											},
+
+											"acr_account_sku": {
+												Type:     pluginsdk.TypeString,
+												Optional: true,
+												ValidateFunc: validation.StringInSlice([]string{
+													"Premium",
+												}, false),
+											},
+
+											"arm_resource_id": {
+												Type:     pluginsdk.TypeString,
+												Computed: true,
+											},
+										},
+									},
+								},
+
+								"user_created_acr_account": {
+									Type:     pluginsdk.TypeList,
+									Optional: true,
+									Elem: &pluginsdk.Resource{
+										Schema: map[string]*pluginsdk.Schema{
+											"arm_resource_id": {
+												Type:         pluginsdk.TypeString,
+												Optional:     true,
+												ValidateFunc: validation.StringIsNotEmpty,
+											},
+										},
+									},
+								},
+							},
+						},
+					},


There's potential to simplify this a lot and make this more user friendly. In what instances is a container registry created for the user by the service? Do they have to supply the sku type that they want?

you are right. users cannot set these fields (system_created_acr_account and user_created_acr_account) in Portal but the API supports them. should we make the whole system_created_acr_account as Computed. if no user_created_acr_account provided then use the system_created_acr_account by default.

stephybun · 2023-08-10T13:12:31Z

internal/services/machinelearning/machine_learning_registry_resource.go

+					"storage_account_details": {
+						Type:     pluginsdk.TypeList,
+						Optional: true,
+						Elem: &pluginsdk.Resource{
+							Schema: map[string]*pluginsdk.Schema{
+								"system_created_storage_account": {
+									Type:     pluginsdk.TypeList,
+									Optional: true,
+									Elem: &pluginsdk.Resource{
+										Schema: map[string]*pluginsdk.Schema{
+											"blob_public_access_allowed": {
+												Type:     pluginsdk.TypeBool,
+												Optional: true,
+												Default:  false,
+											},
+
+											"arm_resource_id": {
+												Type:     pluginsdk.TypeString,
+												Computed: true,
+											},
+
+											"storage_account_name": {
+												Type:     pluginsdk.TypeString,
+												Computed: true,
+											},
+
+											"storage_account_type": {
+												Type:     pluginsdk.TypeString,
+												Optional: true,
+												ValidateFunc: validation.StringInSlice([]string{
+													"Standard_LRS",
+													"Standard_GRS",
+													"Standard_RAGRS",
+													"Standard_ZRS",
+													"Standard_GZRS",
+													"Standard_RAGZRS",
+													"Premium_LRS",
+													"Premium_ZRS",
+												}, false),
+											},
+
+											"storage_account_hns_enabled": {
+												Type:     pluginsdk.TypeBool,
+												Optional: true,
+												Default:  false,
+											},
+										},
+									},
+								},
+
+								"user_created_storage_account": {
+									Type:     pluginsdk.TypeList,
+									Optional: true,
+									Elem: &pluginsdk.Resource{
+										Schema: map[string]*pluginsdk.Schema{
+											"arm_resource_id": {
+												Type:         pluginsdk.TypeString,
+												Required:     true,
+												ValidateFunc: validation.StringIsNotEmpty,
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},


Same question posed above applies here.

stephybun

According to the MSFT learn docs region_details would specify a primary region which cannot be changed after creation as well as additional regions for replication.

There is also a separate endpoint defined in the swagger for removing the additional regions.

Considering the above, the schema needs a significant overhaul to reflect that better and to bring the user experience more in-line with other resources in the provider.

I've left a comment in-line on what the schema needs to look like.

I'd also like to suggest reviewing our property naming guidelines again since many of the names here oppose the customs that we have in the provider today e.g. abbreviating acr -> container registry, appending block names with _details, _config, _properties etc.

internal/services/machinelearning/machine_learning_registry_resource.go

stephybun · 2023-09-13T07:08:05Z

internal/services/machinelearning/machine_learning_registry_resource.go

+	}
+}
+
+func (m MachineLearningRegistryResource) Update() sdk.ResourceFunc {


The additional regions, now renamed to replica_locations in the schema, should be updatable, or in the very least, removable. Can you please investigate?

github-actions · 2023-10-23T06:03:02Z

This PR is being labeled as "stale" because it has not been updated for 30 or more days.

If this PR is still valid, please remove the "stale" label. If this PR is blocked, please add it to the "Blocked" milestone.

If you need some help completing this PR, please leave a comment letting us know. Thank you!

katbyte · 2023-12-06T15:49:26Z

Closing as there has been no action/change to this draft PR for a couple months now. please do reopen if this is ready for review.

github-actions · 2024-05-05T02:10:22Z

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 bot added documentation dependencies service/machine-learning size/XL waiting-response and removed waiting-response labels Aug 9, 2023

stephybun requested changes Aug 10, 2023

View reviewed changes

github-actions bot added waiting-response and removed waiting-response labels Aug 10, 2023

wuxu92 marked this pull request as draft August 21, 2023 02:28

stephybun self-assigned this Sep 12, 2023

stephybun requested changes Sep 13, 2023

View reviewed changes

github-actions bot added waiting-response and removed waiting-response labels Sep 13, 2023

machine learning registry resource

3cb5194

wuxu92 force-pushed the ml/registry branch from 5080c92 to 3cb5194 Compare September 19, 2023 05:33

github-actions bot added waiting-response and removed waiting-response labels Sep 19, 2023

github-actions bot added the stale label Oct 23, 2023

katbyte closed this Dec 6, 2023

github-actions bot removed the stale label Dec 6, 2023

github-actions bot locked as resolved and limited conversation to collaborators May 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: `azurerm_machine_learning_registry` #22870

New Resource: `azurerm_machine_learning_registry` #22870

wuxu92 commented Aug 9, 2023 •

edited

Loading

stephybun left a comment

stephybun Aug 9, 2023

stephybun Aug 9, 2023

stephybun Aug 10, 2023

stephybun Aug 10, 2023

wuxu92 Aug 14, 2023

stephybun Aug 10, 2023

stephybun left a comment

stephybun Sep 13, 2023

github-actions bot commented Oct 23, 2023

katbyte commented Dec 6, 2023

github-actions bot commented May 5, 2024

	return fmt.Errorf("no deadline for ctx")
	return fmt.Errorf("internal-error: no deadline for ctx")

New Resource: azurerm_machine_learning_registry #22870

New Resource: azurerm_machine_learning_registry #22870

Conversation

wuxu92 commented Aug 9, 2023 • edited Loading

stephybun left a comment

Choose a reason for hiding this comment

stephybun Aug 9, 2023

Choose a reason for hiding this comment

stephybun Aug 9, 2023

Choose a reason for hiding this comment

stephybun Aug 10, 2023

Choose a reason for hiding this comment

stephybun Aug 10, 2023

Choose a reason for hiding this comment

wuxu92 Aug 14, 2023

Choose a reason for hiding this comment

stephybun Aug 10, 2023

Choose a reason for hiding this comment

stephybun left a comment

Choose a reason for hiding this comment

stephybun Sep 13, 2023

Choose a reason for hiding this comment

github-actions bot commented Oct 23, 2023

katbyte commented Dec 6, 2023

github-actions bot commented May 5, 2024

New Resource: `azurerm_machine_learning_registry` #22870

New Resource: `azurerm_machine_learning_registry` #22870

wuxu92 commented Aug 9, 2023 •

edited

Loading