New Resource: `azurerm_databricks_workspace` #1134

yaron2 · 2018-04-18T00:13:54Z

This PR adds Azure Databricks Workspace support as "azurerm_databricks_workspace".

achandmsft · 2018-04-24T22:50:07Z

@tombuildsstuff @metacpp could one of you please take a look at this PR so we can ship this?

JunyiYi · 2018-04-25T04:10:24Z

azurerm/resource_arm_databricks_workspace.go

+		return fmt.Errorf("Cannot read Databricks Workspace Instance %s (resource group %s) ID", name, resGroup)
+	}
+
+	log.Printf("[DEBUG] Waiting for Databricks Workspace (%s) to become available", d.Get("workspace_name"))


Why not using name variable here?

JunyiYi · 2018-04-25T04:11:44Z

azurerm/resource_arm_databricks_workspace.go

+
+	future, err := client.CreateOrUpdate(ctx, workspace, resGroup, name)
+	if err != nil {
+		return err


Better to refine the error message here to be consist with other resources.

JunyiYi · 2018-04-25T04:15:24Z

azurerm/resource_arm_databricks_workspace.go

+		return fmt.Errorf("Cannot read Databricks Workspace %s (resource group %s) ID", name, resGroup)
+	}
+
+	log.Printf("[DEBUG] Waiting for Databricks Workspace (%s) to become available", d.Get("workspace_name"))


azurerm/resource_arm_databricks_workspace.go

JunyiYi · 2018-04-25T04:17:26Z

azurerm/resource_arm_databricks_workspace.go

+	resp, err := client.Get(ctx, resGroup, name)
+
+	// covers if the resource has been deleted outside of TF, but is still in the state
+	if resp.StatusCode == http.StatusNotFound {


if utils.ResponseWasNotFound(resp.Response) {

JunyiYi · 2018-04-25T04:19:52Z

azurerm/resource_arm_databricks_workspace.go

+
+	future, err := client.Delete(ctx, resGroup, name)
+	if err != nil {
+		if response.WasNotFound(future.Response()) {


What is this for?

JunyiYi · 2018-04-25T04:25:04Z

Hi @yaron2 , did you upgrade the Go SDK? If so, did you also change the vendor/vendor.json file?

yaron2 · 2018-04-30T17:01:56Z

@JunyiYi Hey, I didn't upgrade the Go SDK

JunyiYi · 2018-04-30T18:49:04Z

@yaron2 , if not, why 4 new files are added under vendor folder (for example, vendor/github.com/Azure/azure-sdk-for-go/services/databricks/mgmt/2018-04-01/databricks/client.go)?

yaron2 · 2018-04-30T21:52:13Z

@JunyiYi Sorry, I thought you meant something else. Yeah I added the vendor for the Databricks ARM support in the Go SDK.

yaron2 · 2018-05-01T18:54:53Z

@JunyiYi Hi, I committed fixes. All yours

JunyiYi · 2018-05-03T23:20:03Z

azurerm/config.go

@@ -534,6 +539,16 @@ func (c *ArmClient) registerContainerServicesClients(endpoint, subscriptionId st
 	c.kubernetesClustersClient = kubernetesClustersClient
 }

+func (c *ArmClient) registerDatabricksClients(endpoint, subscriptionId string, auth autorest.Authorizer, sender autorest.Sender) {
+	databricksWorkspacesClient := databricks.NewWorkspacesClientWithBaseURI(endpoint, subscriptionId)
+	setUserAgent(&databricksWorkspacesClient.Client)


Is it possible to replace these lines of code with configureClient() like the example here.

JunyiYi · 2018-05-03T23:23:20Z

azurerm/resource_arm_databricks_workspace.go

+				ForceNew: true,
+			},
+
+			"location": {


There is a locationSchema() function to declare the location field schema, you can reference the example here.

JunyiYi · 2018-05-03T23:31:16Z

@yaron2 , if you have updated vendor, there should be some changes made in vendor/vendor.json. Take an example for AKS here.

yaron2 · 2018-05-17T03:43:09Z

@JunyiYi updated requested changes. should be good to go..

tombuildsstuff

hey @yaron2

Thanks for this PR / pushing those updates - I've taken a look through and left some comments inline, if we can get those sorted then we should be good to run the tests :)

Thanks!

tombuildsstuff · 2018-05-23T18:38:15Z

azurerm/resource_arm_databricks_workspace.go

+	}
+
+	// covers if the resource has been deleted outside of TF, but is still in the state
+	if utils.ResponseWasNotFound(resp.Response) {


this needs to be moved inside the if err != nil check above, given 404's are now treated as errors

tombuildsstuff · 2018-05-23T18:42:05Z

azurerm/resource_arm_databricks_workspace.go

+		Delete: resourceArmDatabricksWorkspaceDelete,
+
+		Schema: map[string]*schema.Schema{
+			"workspace_name": {


can we update this to just name to match the other resources?

tombuildsstuff · 2018-05-23T18:42:36Z

azurerm/resource_arm_databricks_workspace.go

+				Default:  "standard",
+				ValidateFunc: validation.StringInSlice([]string{
+					"standard",
+					"premium",


are there constants in the SDK that we can use here? i.e. string(databricks.Standard),

tombuildsstuff · 2018-05-23T18:43:01Z

azurerm/resource_arm_databricks_workspace.go

+			"sku": {
+				Type:     schema.TypeString,
+				Required: false,
+				Optional: true,


is there a reason to make this Optional + Defaulted, rather than Required? (e.g. does that match Azure?)

azurerm/resource_arm_databricks_workspace.go

tombuildsstuff · 2018-05-23T19:07:44Z

azurerm/resource_arm_databricks_workspace.go

+
+	future, err := client.Delete(ctx, resGroup, name)
+	if err != nil {
+		return err


can we wrap this error message to make it clearer what failed here? e.g.

return fmt.Errorf("Error deleting Databricks Workspace %q (Resource Group %q): %+v", name, resGroup, err)

tombuildsstuff · 2018-05-23T19:07:58Z

azurerm/resource_arm_databricks_workspace.go

+	return nil
+}
+
+func workspaceStateRefreshFunc(ctx context.Context, client databricks.WorkspacesClient, resourceGroupName string, workspaceName string) resource.StateRefreshFunc {


(as above) I think we can remove this method?

azurerm/resource_arm_databricks_workspace_test.go

azurerm/resource_arm_databricks_workspace.go

tombuildsstuff · 2018-05-23T19:13:47Z

One thing I forgot to comment in the PR - since this is touching a new Resource Provider, we also need to register this in the determineAzureResourceProvidersToRegister method - e.g. add the value Microsoft.Databricks (which is case sensitive)

Thanks!

bcornils · 2018-05-23T21:09:03Z

I see we are blocked on this one @yaron2 or @JunyiYi are you good with the feedback here? Let me know if you need assistance to clear the blockers.

JunyiYi · 2018-06-01T05:31:15Z

@bcornils I agree with @tombuildsstuff 's comments and still waiting for the author to update the code. I think we'd better discuss some strategy (whether the reviewers should update the code for the author) when the original author hasn't been working on it after some specific time.

katbyte · 2018-06-06T01:59:31Z

Hello @yaron2,

Just wondering if your still working on this 🙂

yaron2 · 2018-06-06T02:00:44Z

I am, was just busy for the past couple of weeks. Will get back to it this week. Get Outlook for Android<https://aka.ms/ghei36>

…

________________________________ From: kt <notifications@github.com> Sent: Tuesday, June 5, 2018 6:59:33 PM To: terraform-providers/terraform-provider-azurerm Cc: Yaron Schneider; Mention Subject: Re: [terraform-providers/terraform-provider-azurerm] New Resource: `azurerm_databricks_workspace` (#1134) Hello @yaron2<https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fyaron2&data=02%7C01%7C%7Cbec60a569b264ad8e5b008d5cb512682%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636638471751416206&sdata=TKzL6B3uQJwKgjsmPpZrUdNeljqWif%2F7IqWpbGly8Jo%3D&reserved=0>, Just wondering if your still working on this 🙂 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fterraform-providers%2Fterraform-provider-azurerm%2Fpull%2F1134%23issuecomment-394915356&data=02%7C01%7C%7Cbec60a569b264ad8e5b008d5cb512682%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636638471751416206&sdata=%2F3wOrwgTz0sOkbHETCaXuyvSPS36w9aNQQBJj91RnIc%3D&reserved=0>, or mute the thread<https://nam03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAPilczuj9It7i6xyocjiLox1VAImE4xTks5t5zeFgaJpZM4TZOy7&data=02%7C01%7C%7Cbec60a569b264ad8e5b008d5cb512682%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C636638471751416206&sdata=FmtIUelCm4OgGRMGHSl8wKqKykM2WH%2BthQVYA2c%2FPpc%3D&reserved=0>.

ranieuwe · 2018-06-28T22:04:30Z

@yaron2 what is the status on this? Do you need support?

waiting on changes

andrey-moor · 2018-08-01T00:28:39Z

@yaron2 do you need help with this?

yaron2 · 2018-08-01T00:31:24Z

@andrey-moor Yeah, just moved roles and didn't get enough time to come back to this.
If you could remove the blockers that'd be great.

andrey-moor · 2018-08-04T01:26:34Z

I've just run acceptance tests, and it seems fine. I will give it another look on Monday, but it seems we're unblocked here.

@tombuildsstuff @JunyiYi let me know if I've missed something.

andrey-moor · 2018-08-08T15:53:07Z

@tombuildsstuff @JunyiYi any chance we can try to put it into the next release?

tombuildsstuff

hey @yaron2 @andrey-moor

Thanks for pushing those changes - apologies for the delayed re-review here!

I've taken a look through and this mostly LGTM - since this needed a rebase I've gone ahead and done that (and made the changes requested in this PR so that we can ship this) - I hope you don't mind! I'm just kicking off the tests now - but we'll try to land this in 1.17 :)

Thanks!

azurerm/resource_arm_databricks_workspace.go

azurerm/resource_arm_databricks_workspace_test.go

vendor/vendor.json

azurerm/resource_arm_databricks_workspace_test.go

azurerm/resource_arm_databricks_workspace.go

``` $ acctests azurerm TestAzureRMDatabrickWorkspaceName === RUN TestAzureRMDatabrickWorkspaceName --- PASS: TestAzureRMDatabrickWorkspaceName (0.00s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 1.270s ```

tombuildsstuff · 2018-10-01T11:07:12Z

Tests pass:

$ acctests azurerm TestAccAzureRMDatabricksWorkspace_
=== RUN   TestAccAzureRMDatabricksWorkspace_basic
--- PASS: TestAccAzureRMDatabricksWorkspace_basic (667.42s)
=== RUN   TestAccAzureRMDatabricksWorkspace_withTags
--- PASS: TestAccAzureRMDatabricksWorkspace_withTags (725.72s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	1394.488s

Lachlan-White · 2019-03-05T02:00:37Z

Hey @tombuildsstuff is there currently plans to have this updated to include the private virtual network options with databricks?

Lachlan-White · 2019-03-05T02:05:50Z

or should i raise a feature request for this?

tombuildsstuff · 2019-03-05T07:12:26Z

@a138076 not that I can see at this time - please open a new feature request for this :)

Thanks!

tombuildsstuff added service/databricks new-resource labels Apr 18, 2018

metacpp requested review from metacpp, WodansSon and JunyiYi April 24, 2018 23:12

JunyiYi suggested changes Apr 25, 2018

View reviewed changes

JunyiYi previously requested changes May 3, 2018

View reviewed changes

WodansSon previously approved these changes May 7, 2018

View reviewed changes

tombuildsstuff requested changes May 23, 2018

View reviewed changes

tombuildsstuff changed the title ~~New resource: added azure databricks support~~ New Resource: azurerm_databricks_workspace May 23, 2018

tombuildsstuff added the waiting-response label May 23, 2018

tombuildsstuff removed the request for review from metacpp July 16, 2018 12:02

tombuildsstuff removed the waiting-response label Oct 1, 2018

tombuildsstuff unassigned JunyiYi Oct 1, 2018

yaron2 and others added 12 commits October 1, 2018 23:40

added databricks support

33c1414

fixed formatting and removed redundant lines

1a65d5f

now using configureClient in registerDatabricksClients

5dfd62e

changed redundant d.Get(workspace_name) to name

de9f40f

changed location to use locationSchema()

7b62daa

updated vendor with azure databricks sdk

132bb37

Address PR feedback

8dc78a2

Register Databricks provider

4ce361f

Fix formatting

3da6f15

Fix acceptance tests

7c6079d

Fixing comments made in the PR

3aa5812

Upgrading to v21.1.0 of the Azure SDK

b2c57d1

tombuildsstuff added this to the 1.17.0 milestone Oct 1, 2018

tombuildsstuff approved these changes Oct 1, 2018

View reviewed changes

tombuildsstuff reviewed Oct 1, 2018

View reviewed changes

azurerm/resource_arm_databricks_workspace.go Outdated Show resolved Hide resolved

Validation for the Databricks Workspace name

d753ac6

``` $ acctests azurerm TestAzureRMDatabrickWorkspaceName === RUN TestAzureRMDatabrickWorkspaceName --- PASS: TestAzureRMDatabrickWorkspaceName (0.00s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 1.270s ```

tombuildsstuff force-pushed the databricks branch from 4854161 to d753ac6 Compare October 1, 2018 10:54

ghost added the size/XXL label Oct 1, 2018

Fixing a merge conflict

19d2cd3

tombuildsstuff merged commit 3bb2097 into hashicorp:master Oct 1, 2018

tombuildsstuff added a commit that referenced this pull request Oct 1, 2018

Updating to include #1134

a83e807

hashicorp locked as resolved and limited conversation to collaborators Mar 5, 2019

New Resource: azurerm_databricks_workspace #1134

New Resource: azurerm_databricks_workspace #1134

Conversation

yaron2 commented Apr 18, 2018 • edited by tombuildsstuff

achandmsft commented Apr 24, 2018

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

JunyiYi commented Apr 25, 2018

yaron2 commented Apr 30, 2018

JunyiYi commented Apr 30, 2018

yaron2 commented Apr 30, 2018

yaron2 commented May 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunyiYi commented May 3, 2018

yaron2 commented May 17, 2018

tombuildsstuff 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

tombuildsstuff commented May 23, 2018

bcornils commented May 23, 2018

JunyiYi commented Jun 1, 2018

katbyte commented Jun 6, 2018

yaron2 commented Jun 6, 2018 via email

ranieuwe commented Jun 28, 2018

andrey-moor commented Aug 1, 2018

yaron2 commented Aug 1, 2018

andrey-moor commented Aug 4, 2018

andrey-moor commented Aug 8, 2018 • edited

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Oct 1, 2018

Lachlan-White commented Mar 5, 2019

Lachlan-White commented Mar 5, 2019

tombuildsstuff commented Mar 5, 2019

New Resource: `azurerm_databricks_workspace` #1134

New Resource: `azurerm_databricks_workspace` #1134

yaron2 commented Apr 18, 2018 •

edited by tombuildsstuff

andrey-moor commented Aug 8, 2018 •

edited