-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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_databricks_workspace
#1134
Conversation
@tombuildsstuff @metacpp could one of you please take a look at this PR so we can ship this? |
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")) |
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.
Why not using name
variable here?
|
||
future, err := client.CreateOrUpdate(ctx, workspace, resGroup, name) | ||
if err != nil { | ||
return err |
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.
Better to refine the error message here to be consist with other resources.
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")) |
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.
name
?
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 { |
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.
if utils.ResponseWasNotFound(resp.Response) {
|
||
future, err := client.Delete(ctx, resGroup, name) | ||
if err != nil { | ||
if response.WasNotFound(future.Response()) { |
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.
What is this for?
Hi @yaron2 , did you upgrade the Go SDK? If so, did you also change the |
@JunyiYi Hey, I didn't upgrade the Go SDK |
@yaron2 , if not, why 4 new files are added under |
@JunyiYi Sorry, I thought you meant something else. Yeah I added the vendor for the Databricks ARM support in the Go SDK. |
@JunyiYi Hi, I committed fixes. All yours |
azurerm/config.go
Outdated
@@ -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) |
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.
Is it possible to replace these lines of code with configureClient()
like the example here.
ForceNew: true, | ||
}, | ||
|
||
"location": { |
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.
There is a locationSchema()
function to declare the location field schema, you can reference the example here.
@yaron2 , if you have updated |
@JunyiYi updated requested changes. should be good to go.. |
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.
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!
} | ||
|
||
// covers if the resource has been deleted outside of TF, but is still in the state | ||
if utils.ResponseWasNotFound(resp.Response) { |
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.
this needs to be moved inside the if err != nil
check above, given 404's are now treated as errors
Delete: resourceArmDatabricksWorkspaceDelete, | ||
|
||
Schema: map[string]*schema.Schema{ | ||
"workspace_name": { |
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.
can we update this to just name
to match the other resources?
Default: "standard", | ||
ValidateFunc: validation.StringInSlice([]string{ | ||
"standard", | ||
"premium", |
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.
are there constants in the SDK that we can use here? i.e. string(databricks.Standard),
"sku": { | ||
Type: schema.TypeString, | ||
Required: false, | ||
Optional: true, |
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.
is there a reason to make this Optional + Defaulted, rather than Required? (e.g. does that match Azure?)
|
||
future, err := client.Delete(ctx, resGroup, name) | ||
if err != nil { | ||
return err |
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.
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)
return nil | ||
} | ||
|
||
func workspaceStateRefreshFunc(ctx context.Context, client databricks.WorkspacesClient, resourceGroupName string, workspaceName string) resource.StateRefreshFunc { |
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.
(as above) I think we can remove this method?
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 Thanks! |
azurerm_databricks_workspace
@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. |
Hello @yaron2, Just wondering if your still working on this 🙂 |
@yaron2 what is the status on this? Do you need support? |
waiting on changes
@yaron2 do you need help with this? |
@andrey-moor Yeah, just moved roles and didn't get enough time to come back to this. |
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. |
@tombuildsstuff @JunyiYi any chance we can try to put it into the next release? |
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.
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!
``` $ acctests azurerm TestAzureRMDatabrickWorkspaceName === RUN TestAzureRMDatabrickWorkspaceName --- PASS: TestAzureRMDatabrickWorkspaceName (0.00s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 1.270s ```
4854161
to
d753ac6
Compare
Tests pass:
|
Hey @tombuildsstuff is there currently plans to have this updated to include the private virtual network options with databricks? |
or should i raise a feature request for this? |
@a138076 not that I can see at this time - please open a new feature request for this :) Thanks! |
This PR adds Azure Databricks Workspace support as "azurerm_databricks_workspace".
Fixes #1133