New Resource: azurerm_machine_learning_workspace #5696

cbellee · 2020-02-12T08:03:30Z

Terraform resource provider to create a basic Azure Machine Learning Workspace.

Test cases successfully passed

=== RUN TestAccAzureRMMachineLearningWorkspace_basic
=== PAUSE TestAccAzureRMMachineLearningWorkspace_basic
=== CONT TestAccAzureRMMachineLearningWorkspace_basic
=== RUN TestAccAzureRMMachineLearningWorkspace_requiresImport
=== PAUSE TestAccAzureRMMachineLearningWorkspace_requiresImport
=== CONT TestAccAzureRMMachineLearningWorkspace_requiresImport
=== RUN TestAccAzureRMMachineLearningWorkspace_withTags
=== PAUSE TestAccAzureRMMachineLearningWorkspace_withTags
=== CONT TestAccAzureRMMachineLearningWorkspace_withTags
=== RUN TestAccAzureRMMachineLearningWorkspace_withContainerRegistry
=== PAUSE TestAccAzureRMMachineLearningWorkspace_withContainerRegistry
=== CONT TestAccAzureRMMachineLearningWorkspace_withContainerRegistry
=== RUN TestAccAzureRMMachineLearningWorkspace_complete
=== PAUSE TestAccAzureRMMachineLearningWorkspace_complete
=== CONT TestAccAzureRMMachineLearningWorkspace_complete
--- PASS: TestAccAzureRMMachineLearningWorkspace_complete (547.09s)
--- PASS: TestAccAzureRMMachineLearningWorkspace_basic (593.96s)
--- PASS: TestAccAzureRMMachineLearningWorkspace_withContainerRegistry (610.00s)
--- PASS: TestAccAzureRMMachineLearningWorkspace_requiresImport (701.89s)
--- PASS: TestAccAzureRMMachineLearningWorkspace_withTags (836.34s)
PASS

ArcturusZhang · 2020-02-12T11:16:00Z

This PR depends on the SDK v39.0.0

katbyte · 2020-03-10T06:40:01Z

@cbellee, @ArcturusZhang - i've opened a PR (#6049) to upgrade the SDK as we typically do that separately on their own. 2.1 should go out in the next couple days and this will get merged right after.

ArcturusZhang · 2020-03-10T08:20:57Z

@cbellee, @ArcturusZhang - i've opened a PR (#6049) to upgrade the SDK as we typically do that separately on their own. 2.1 should go out in the next couple days and this will get merged right after.

Sure, I will rebase this PR once PR #6049 get merged

This PR supersedes #6049 as nested go modules and merge conflicts do not spark joy - but fundamentally this: - updates github.com/Azure/azure-sdk-for-go to v40.3.0 - updates github.com/Azure/go-autorest to our fork containing Azure/go-autorest#512 - updates github.com/terraform-providers/terraform-provider-azuread to v0.8.0 - code changes needed for v40.3.0 of the Azure SDK - including opting into the old count 429's as requests which should be retried without adding to the total failure count Enables #5769 Enables #5696

katbyte

Hi @cbellee,

thanks for the new resource! this is off to a great start and most of my comments are pretty minor. Once they are addressed this should be good ot merge!

katbyte · 2020-03-21T00:45:27Z

azurerm/internal/services/machinelearning/resource_arm_machine_learning_workspace.go

+	_, err = client.Update(ctx, id.ResourceGroup, id.Name, update)
+	if err != nil {


could we merge these two lines?

katbyte · 2020-03-21T00:45:36Z

azurerm/internal/services/machinelearning/resource_arm_machine_learning_workspace.go

+	_, err = client.Delete(ctx, id.ResourceGroup, id.Name)
+	if err != nil {


could we merge these two lines?

website/docs/d/machine_learning_workspace.html.markdown

katbyte · 2020-03-21T00:51:26Z

website/docs/r/machine_learning_workspace.html.markdown

+
+* `friendly_name` - (Optional) Friendly name for this Machine Learning Workspace.
+
+* `sku_name` - (Optional) SKU/edition of the Machine Learning Workspace, possible values are `Basic` for a basic workspace or `Enterprise` for a feature rich workspace. Default to `Basic`.


Suggested change

* `sku_name` - (Optional) SKU/edition of the Machine Learning Workspace, possible values are `Basic` for a basic workspace or `Enterprise` for a feature rich workspace. Default to `Basic`.

* `sku_name` - (Optional) SKU/edition of the Machine Learning Workspace, possible values are `Basic` for a basic workspace or `Enterprise` for a feature rich workspace. Defaults to `Basic`.

katbyte · 2020-03-21T00:53:40Z

azurerm/internal/services/machinelearning/tests/resource_arm_machine_learning_workspace_test.go

+	})
+}
+
+func TestAccAzureRMMachineLearningWorkspace_withTags(t *testing.T) {


We can just merge this with the complete test (and i notice that test isn't setting the tags, it should set as many of the properties as possible)

azurerm/internal/services/machinelearning/tests/resource_arm_machine_learning_workspace_test.go

azurerm/internal/services/machinelearning/tests/data_source_machine_learning_workspace_test.go

katbyte · 2020-03-21T00:55:31Z

azurerm/internal/services/machinelearning/resource_arm_machine_learning_workspace.go

+		description := d.Get("description").(string)
+		update.WorkspacePropertiesUpdateParameters.Description = &description


minor these could be

Suggested change

description := d.Get("description").(string)

update.WorkspacePropertiesUpdateParameters.Description = &description

update.WorkspacePropertiesUpdateParameters.Description = &utils.String(d.Get("description").(string))

Done, and I also changed some other field assignment with similar pattern

ArcturusZhang · 2020-03-21T03:01:17Z

Hi @katbyte I have resolved your comments.
As for the acc test, for this resource we cannot do an update test by basic -> complete -> basic, because the complete test set the container_registry_id attribute which is ForceNew, this may make the update stop making sense. Therefore I added an update step for both the basic and complete test.

And according to Tom's comments in other PR that I spotted recently, I have removed the features.ShouldResourcesBeImported() and d.IsNewResource() check in Create function.

Plus, since I am neither the author of this PR nor reviewer, I cannot resolve those comment thread, hope you do not mind.

katbyte

Thanks for the revisions @ArcturusZhang,

In addition to the comments i've left inline we have a test failure:

Test Failed

------- Stdout: -------
=== RUN   TestAccDataSourceAzureRMMachineLearningWorkspace_basic
=== PAUSE TestAccDataSourceAzureRMMachineLearningWorkspace_basic
=== CONT  TestAccDataSourceAzureRMMachineLearningWorkspace_basic
--- FAIL: TestAccDataSourceAzureRMMachineLearningWorkspace_basic (332.82s)
    testing.go:640: Step 1 error: 11 problems:
        
        - Provider produced invalid object: Provider "azurerm" planned an invalid value for data.azurerm_machine_learning_workspace.test during refresh: unsupported attribute "identity".
        
        This is a bug in the provider, which should be reported in the provider's own issue tracker.
        - Provider produced invalid object: Provider "azurerm" planned an invalid value for data.azurerm_machine_learning_workspace.test during refresh: unsupported attribute "key_vault_id".
        
        This is a bug in the provider, which should be reported in the provider's own issue tracker.
        - Provider produced invalid object: Provider "azurerm" planned an invalid value for data.azurerm_machine_learning_workspace.test during refresh: unsupported attribute "application_insights_id".
        
        This is a bug in the provider, which should be reported in the provider's own issue tracker.
        - Provider produced invalid object: Provider "azurerm" planned an invalid value for data.azurerm_machine_learning_workspace.test during refresh: unsupported attribute "description".
        
        This is a bug in the provider, which should be reported in the provider's own issue tracker.
        - Provider produced invalid object: Provider "azurerm" planned an invalid value for data.azurerm_machine_learning_workspace.test during refresh: unsupported attribute "sku_name".
        
        This is a bug in the provider, which should be reported in the provider's own issue tracker.
        - Provider produced invalid object: Provider "azurerm" planned an invalid value for data.azurerm_machine_learning_workspace.test during refresh: unsupported attribute "friendly_name".
        
        This is a bug in the provider, which should be reported in the provider's own issue tracker.
        - Provider produced invalid object: Provider "azurerm" planned an invalid value for data.azurerm_machine_learning_workspace.test during refresh: unsupported attribute "storage_account_id".
        
        This is a bug in the provider, which should be reported in the provider's own issue tracker.
        - Provider produced invalid object: Provider "azurerm" planned an invalid value for data.azurerm_machine_learning_workspace.test during refresh: unsupported attribute "container_registry_id".
        
        This is a bug in the provider, which should be reported in the provider's own issue tracker.
        - Provider produced invalid object: Provider "azurerm" planned an invalid value for data.azurerm_machine_learning_workspace.test during refresh: .timeouts: unsupported attribute "update".
        
        This is a bug in the provider, which should be reported in the provider's own issue tracker.
        - Provider produced invalid object: Provider "azurerm" planned an invalid value for data.azurerm_machine_learning_workspace.test during refresh: .timeouts: unsupported attribute "create".
        
        This is a bug in the provider, which should be reported in the provider's own issue tracker.
        - Provider produced invalid object: Provider "azurerm" planned an invalid value for data.azurerm_machine_learning_workspace.test during refresh: .timeouts: unsupported attribute "delete".
        
        This is a bug in the provider, which should be reported in the provider's own issue tracker.

katbyte · 2020-03-23T23:03:11Z

azurerm/internal/services/machinelearning/resource_arm_machine_learning_workspace.go

+		Location: &location,
+		Tags:     tags.Expand(t),
+		Sku:      &machinelearningservices.Sku{Name: utils.String(skuName)},
+		Identity: identity,


Given that this is always required, i think we could

Suggested change

Identity: identity,

Identity: utils.String(d.Get("identity.0.type").(string)),

Well, identity is a block, therefore I bring the definition of identity here in the struct initialization

katbyte · 2020-03-23T23:03:29Z

azurerm/internal/services/machinelearning/resource_arm_machine_learning_workspace.go

+			"sku_name": {
+				Type:     schema.TypeString,
+				Optional: true,
+				ValidateFunc: validation.StringInSlice([]string{


could we put default just before the validation function?

azurerm/internal/services/machinelearning/data_source_machine_learning_workspace.go

katbyte · 2020-03-23T23:09:04Z

azurerm/internal/services/machinelearning/tests/resource_arm_machine_learning_workspace_test.go

+%s
+
+resource "azurerm_machine_learning_workspace" "test" {
+  name                    = "acctestworkspace-%d"


could we include capitals in every property that allows them:

Suggested change

name = "acctestworkspace-%d"

name = "acctest-MLW-%d"

website/docs/r/machine_learning_workspace.html.markdown

ArcturusZhang · 2020-03-24T01:22:27Z

Hi @katbyte I have resolved the new comments, and find I missed the provider block in acc test configs and example usage in docs (already added them)
The test failure has been fixed now. Please have a look, thanks!

katbyte

Thanks for the fixed @ArcturusZhang, tests pass now! LGTM 👍

website/docs/r/machine_learning_workspace.html.markdown

katbyte · 2020-03-24T04:13:24Z

azurerm/internal/services/machinelearning/resource_arm_machine_learning_workspace.go

+func expandArmMachineLearningWorkspaceIdentity(input []interface{}) *machinelearningservices.Identity {
+	if len(input) == 0 {
+		return nil
+	}
+
+	v := input[0].(map[string]interface{})
+
+	identityType := machinelearningservices.ResourceIdentityType(v["type"].(string))
+
+	identity := machinelearningservices.Identity{
+		Type: identityType,
+	}
+
+	return &identity
+}


It was more if you did the complex d.Get you could have removed this entire function simplifying the code. minor so not going to block the PR on it

I see, thank you!

ghost · 2020-03-27T11:04:00Z

This has been released in version 2.3.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.3.0"
}
# ... other configuration ...

ghost · 2020-04-23T14:50:07Z

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!

ghost added the size/XXL label Feb 12, 2020

ArcturusZhang force-pushed the f/machine_learning_services branch from 4aa6624 to 0e3b15e Compare February 12, 2020 10:26

ghost added the documentation label Feb 12, 2020

ArcturusZhang force-pushed the f/machine_learning_services branch from 0e3b15e to ec02938 Compare February 12, 2020 10:30

ghost added the dependencies label Feb 12, 2020

ArcturusZhang force-pushed the f/machine_learning_services branch 3 times, most recently from 863c148 to 228dced Compare February 14, 2020 09:35

ArcturusZhang force-pushed the f/machine_learning_services branch 2 times, most recently from 1d4c4fc to 315fe8c Compare March 2, 2020 04:43

cbellee and others added 5 commits March 6, 2020 18:13

updated azure-sdk-for-go to v39.0.0+incompatible

b7c24bd

Changes from Dapeng

510d4d5

Fix CI failures

952bd3b

Changes to conform 2.0

e60dd7e

Remove bad links to fix CI

b60d268

ArcturusZhang force-pushed the f/machine_learning_services branch from 5cbbe12 to b60d268 Compare March 6, 2020 10:14

Fix format

9d4219c

katbyte added the sdk/requires-upgrade This is dependent upon upgrading an SDK label Mar 6, 2020

katbyte mentioned this pull request Mar 10, 2020

dependencies: upgrade azure-sdk-for-go to v40.3.0 #6049

Closed

tombuildsstuff mentioned this pull request Mar 17, 2020

Updating the Azure SDK to v40.3.0 / AzureAD to v0.8.0 #6134

Merged

ArcturusZhang added 2 commits March 19, 2020 11:54

Merge branch 'master' into f/machine_learning_services

50909ed

Merge branch 'master' into f/machine_learning_services

9c9338f

katbyte requested changes Mar 21, 2020

View reviewed changes

katbyte added service/machine-learning waiting-response labels Mar 21, 2020

Resolve comments

89bf14b

ghost removed the waiting-response label Mar 21, 2020

katbyte requested changes Mar 23, 2020

View reviewed changes

katbyte added waiting-response and removed sdk/requires-upgrade This is dependent upon upgrading an SDK labels Mar 23, 2020

Resolve comments

a50a8da

ghost removed the waiting-response label Mar 24, 2020

Goimports

132a84f

katbyte approved these changes Mar 24, 2020

View reviewed changes

katbyte changed the title ~~F/machine learning services~~ New Resource: azurerm_machine_learning_workspace Mar 24, 2020

katbyte added this to the v2.3.0 milestone Mar 24, 2020

katbyte merged commit f903b2f into hashicorp:master Mar 24, 2020

katbyte added a commit that referenced this pull request Mar 24, 2020

update CHANGELOG,md to include #5696

edf4033

ArcturusZhang deleted the f/machine_learning_services branch March 24, 2020 05:51

tombuildsstuff linked an issue Apr 1, 2020 that may be closed by this pull request

New Resource: Azure Machine Learning Service #3729

Closed

hashicorp locked and limited conversation to collaborators Apr 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: azurerm_machine_learning_workspace #5696

New Resource: azurerm_machine_learning_workspace #5696

cbellee commented Feb 12, 2020

ArcturusZhang commented Feb 12, 2020

katbyte commented Mar 10, 2020

ArcturusZhang commented Mar 10, 2020

katbyte left a comment

katbyte Mar 21, 2020

ArcturusZhang Mar 21, 2020

katbyte Mar 21, 2020

ArcturusZhang Mar 21, 2020

katbyte Mar 21, 2020

ArcturusZhang Mar 21, 2020

katbyte Mar 21, 2020

ArcturusZhang Mar 21, 2020

katbyte Mar 21, 2020

ArcturusZhang Mar 21, 2020

ArcturusZhang commented Mar 21, 2020

katbyte left a comment

katbyte Mar 23, 2020

ArcturusZhang Mar 23, 2020

katbyte Mar 23, 2020

ArcturusZhang Mar 23, 2020

katbyte Mar 23, 2020

ArcturusZhang Mar 23, 2020

ArcturusZhang commented Mar 24, 2020

katbyte left a comment

katbyte Mar 24, 2020

ArcturusZhang Mar 24, 2020

ghost commented Mar 27, 2020

ghost commented Apr 23, 2020

		_, err = client.Update(ctx, id.ResourceGroup, id.Name, update)
		if err != nil {

		_, err = client.Delete(ctx, id.ResourceGroup, id.Name)
		if err != nil {


		* `friendly_name` - (Optional) Friendly name for this Machine Learning Workspace.

		* `sku_name` - (Optional) SKU/edition of the Machine Learning Workspace, possible values are `Basic` for a basic workspace or `Enterprise` for a feature rich workspace. Default to `Basic`.

		description := d.Get("description").(string)
		update.WorkspacePropertiesUpdateParameters.Description = &description

	Identity: identity,
	Identity: utils.String(d.Get("identity.0.type").(string)),

New Resource: azurerm_machine_learning_workspace #5696

New Resource: azurerm_machine_learning_workspace #5696

Conversation

cbellee commented Feb 12, 2020

ArcturusZhang commented Feb 12, 2020

katbyte commented Mar 10, 2020

ArcturusZhang commented Mar 10, 2020

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArcturusZhang commented Mar 21, 2020

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArcturusZhang commented Mar 24, 2020

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 27, 2020

ghost commented Apr 23, 2020