-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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_virtual_network_peering
#20728
Conversation
moving this out one release to allow time for fixes in the SDK to be merged for the delete issue |
The Base Layer will be updated once hashicorp/pandora#2241 is merged, but since the workaround works, we can ship this for now and then clean this up once the updated SDK is available later this/early next week. |
internal/services/databricks/databricks_virtual_network_peerings_resource.go
Outdated
Show resolved
Hide resolved
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 @WodansSon
Thanks for this PR - I've taken a look through and left some comments to both make this more resilient and bring this into line with other resources across the Provider, if we can fix those up then we should be able to take another look.
it's worth noting that the updated base layer is now available in the main
branch since #21004 has been merged, which should resolve a couple of the issues commented in this PR.
Thanks!
VnetPeeringsClient := vnetpeering.NewVNetPeeringClientWithBaseURI(o.ResourceManagerEndpoint) | ||
o.ConfigureClient(&VnetPeeringsClient.Client, o.ResourceManagerAuthorizer) |
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.
FYI #21004 introduces the new base layer, so this'll want to become:
VnetPeeringsClient := vnetpeering.NewVNetPeeringClientWithBaseURI(o.ResourceManagerEndpoint) | |
o.ConfigureClient(&VnetPeeringsClient.Client, o.ResourceManagerAuthorizer) | |
vnetPeeringsClient := vnetpeering.NewVNetPeeringClientWithBaseURI(o.Environment.ResourceManager) | |
if err != nil { | |
return nil, fmt.Errorf("building VNetPeering client: %+v", err) | |
} | |
o.Configure(vnetPeeringsClient.Client, o.Authorizers.ResourceManager) |
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.
Fixed.
|
||
return &Client{ | ||
AccessConnectorClient: &AccessConnectorClient, | ||
WorkspacesClient: &WorkspacesClient, | ||
VnetPeeringsClient: &VnetPeeringsClient, |
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.
FYI #21004 introduces the new base layer, so this'll want to become:
VnetPeeringsClient: &VnetPeeringsClient, | |
VnetPeeringsClient: vnetPeeringsClient, |
(I'm intentionally not touching AccessConnectorClient
and WorkspacesClient
above to avoid a merge conflict, fwiw - those are changed as a part of #21004)
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.
Fixed.
peerMutex.Lock(resourceLock) | ||
defer peerMutex.Unlock(resourceLock) |
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 lock should be at the top of the create function, else the RequiresImport
check won't be effective?
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.
Fixed.
// The RP always creates the same vNet ID for the Databricks internal vNet in the below format: | ||
// '/subscriptions/{subscription}/resourceGroups/{group1}/providers/Microsoft.Network/virtualNetworks/workers-vnet' | ||
fmtString := "/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/workers-vnet" | ||
virtualNetworkId := fmt.Sprintf(fmtString, id.SubscriptionId, id.ResourceGroupName) |
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.
we shouldn't be hard-coding Resource ID strings like this - we can use the Resource ID types to generate this without hard-coding the string here:
// The RP always creates the same vNet ID for the Databricks internal vNet in the below format: | |
// '/subscriptions/{subscription}/resourceGroups/{group1}/providers/Microsoft.Network/virtualNetworks/workers-vnet' | |
fmtString := "/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Network/virtualNetworks/workers-vnet" | |
virtualNetworkId := fmt.Sprintf(fmtString, id.SubscriptionId, id.ResourceGroupName) | |
virtualNetworkId := networkParse.NewVirtualNetworkID(id.SubscriptionId, id.ResourceGroupName, "workers-vnet").ID() |
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.
Fixed.
Create: resourceDatabricksVirtualNetworkPeeringCreateUpdate, | ||
Read: resourceDatabricksVirtualNetworkPeeringRead, | ||
Update: resourceDatabricksVirtualNetworkPeeringCreateUpdate, |
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.
since there's differences in the codepaths (below) can we split the Create and Update method
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.
Fixed.
|
||
The following arguments are supported: | ||
|
||
* `name` - (Required) Specifies the name of the Databricks Virtual Network Peering resource. Possible valid values must begin with a letter or number, end with a letter, number or underscore, and may contain only letters, numbers, underscores, periods, or hyphens and must be between 1 and 80 characters in length. Changing this forces a new resource to be created. |
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.
we don't tend to call out the validation requirements for string length etc here, the validate
funcs handle this, so we can simplify this to:
* `name` - (Required) Specifies the name of the Databricks Virtual Network Peering resource. Possible valid values must begin with a letter or number, end with a letter, number or underscore, and may contain only letters, numbers, underscores, periods, or hyphens and must be between 1 and 80 characters in length. Changing this forces a new resource to be created. | |
* `name` - (Required) Specifies the name of the Databricks Virtual Network Peering resource. Changing this forces a new resource to be created. |
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.
Fixed.
|
||
* `resource_group_name` - (Required) The name of the Resource Group in which the Databricks Virtual Network Peering should exist. Changing this forces a new resource to be created. | ||
|
||
* `workspace_id` - (Required) The Id of the Databricks Workspace that this Databricks Virtual Network Peering is bound. Changing this forces a new resource to be created. |
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.
minor casing:
* `workspace_id` - (Required) The Id of the Databricks Workspace that this Databricks Virtual Network Peering is bound. Changing this forces a new resource to be created. | |
* `workspace_id` - (Required) The ID of the Databricks Workspace that this Databricks Virtual Network Peering is bound. Changing this forces a new resource to be created. |
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.
Fixed.
|
||
~> **NOTE:** If the `use_remote_gateways` is set to `true`, and `allow_gateway_transit` on the remote peering is also `true`, the virtual network will use the gateways of the remote virtual network for transit. Only one peering can have this flag set to `true`. `use_remote_gateways` cannot be set if the virtual network already has a gateway. | ||
|
||
* `virtual_network_id` - (Computed) The Id of the internal databricks virtual network. |
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.
we have the Attributes
section to highlight exported attributes, where this can become:
* `virtual_network_id` - (Computed) The Id of the internal databricks virtual network. | |
* `virtual_network_id` - The ID of the internal Virtual Network used by the DataBricks Workspace. |
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.
Fixed.
|
||
In addition to the Arguments listed above - the following Attributes are exported: | ||
|
||
* `id` - The ID of the Databricks Virtual Network Peering in the Azure management plane. |
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 can become:
* `id` - The ID of the Databricks Virtual Network Peering in the Azure management plane. | |
* `id` - The ID of the Databricks Virtual Network Peering. |
(we don't use the phrase Management plane
since it's used interchangably with several other phrases in the Azure docs, and would be superfluous here)
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.
Fixed.
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 @WodansSon
Thanks for this PR - I've taken a look through and left some comments to both make this more resilient and bring this into line with other resources across the Provider, if we can fix those up then we should be able to take another look.
it's worth noting that the updated base layer is now available in the main
branch since #21004 has been merged, which should resolve a couple of the issues commented in this PR.
Thanks!
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.
Noticed one thing whilst investigating the related bug hashicorp/go-azure-sdk#381, see inline comment below.
I've opened hashicorp/go-azure-sdk#383 to fix the unmarshaling bug during the Delete operation, once this is merged it'll push a new go-azure-sdk version that you can vendor in.
future, err := client.Delete(ctx, *id) | ||
if err != nil { | ||
return fmt.Errorf("deleting Databricks %s: %+v", *id, err) | ||
} | ||
|
||
if err = future.Poller.PollUntilDone(ctx); err != nil { | ||
return fmt.Errorf("waiting for deletion of Databricks %s: %+v", *id, 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 this be reduced to a single call to client.DeleteThenPoll()
?
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.
@manicminer, I noticed that too in the debug, this was calling delete twice... Fixed.
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 @WodansSon
Thanks for pushing those changes - taking a look through this is looking mostly good - I've left some more comments inline, but once we fix those up (and rebase this on top of main
, which by the time you see this should have the SDK updated to include hashicorp/go-azure-sdk#383) then this should be good to go 👍
Thanks!
existing, err := client.Get(ctx, *id) | ||
if err != nil { | ||
return fmt.Errorf("retrieving Databricks %s: %+v", *id, err) | ||
} | ||
|
||
// these are the only updatable values, so everything else in the existing.Model should still be unchanged... | ||
if d.HasChange("allow_forwarded_traffic") { | ||
existing.Model.Properties.AllowForwardedTraffic = pointer.To(d.Get("allow_forwarded_traffic").(bool)) | ||
} | ||
|
||
if d.HasChange("allow_gateway_transit") { | ||
existing.Model.Properties.AllowGatewayTransit = pointer.To(d.Get("allow_gateway_transit").(bool)) | ||
} | ||
|
||
if d.HasChange("allow_virtual_network_access") { | ||
existing.Model.Properties.AllowVirtualNetworkAccess = pointer.To(d.Get("allow_virtual_network_access").(bool)) | ||
} | ||
|
||
if d.HasChange("use_remote_gateways") { | ||
existing.Model.Properties.UseRemoteGateways = pointer.To(d.Get("use_remote_gateways").(bool)) | ||
} | ||
|
||
locks.ByID(databricksVnetPeeringsResourceType) | ||
defer locks.UnlockByID(databricksVnetPeeringsResourceType) |
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.
these two lines needs to be moved up to before we make the Get call, else if another Resource modifies the DataBricks cluster we could be undoing that change?
existing, err := client.Get(ctx, *id) | |
if err != nil { | |
return fmt.Errorf("retrieving Databricks %s: %+v", *id, err) | |
} | |
// these are the only updatable values, so everything else in the existing.Model should still be unchanged... | |
if d.HasChange("allow_forwarded_traffic") { | |
existing.Model.Properties.AllowForwardedTraffic = pointer.To(d.Get("allow_forwarded_traffic").(bool)) | |
} | |
if d.HasChange("allow_gateway_transit") { | |
existing.Model.Properties.AllowGatewayTransit = pointer.To(d.Get("allow_gateway_transit").(bool)) | |
} | |
if d.HasChange("allow_virtual_network_access") { | |
existing.Model.Properties.AllowVirtualNetworkAccess = pointer.To(d.Get("allow_virtual_network_access").(bool)) | |
} | |
if d.HasChange("use_remote_gateways") { | |
existing.Model.Properties.UseRemoteGateways = pointer.To(d.Get("use_remote_gateways").(bool)) | |
} | |
locks.ByID(databricksVnetPeeringsResourceType) | |
defer locks.UnlockByID(databricksVnetPeeringsResourceType) | |
locks.ByID(databricksVnetPeeringsResourceType) | |
defer locks.UnlockByID(databricksVnetPeeringsResourceType) | |
existing, err := client.Get(ctx, *id) | |
if err != nil { | |
return fmt.Errorf("retrieving Databricks %s: %+v", *id, err) | |
} | |
// these are the only updatable values, so everything else in the existing.Model should still be unchanged... | |
if d.HasChange("allow_forwarded_traffic") { | |
existing.Model.Properties.AllowForwardedTraffic = pointer.To(d.Get("allow_forwarded_traffic").(bool)) | |
} | |
if d.HasChange("allow_gateway_transit") { | |
existing.Model.Properties.AllowGatewayTransit = pointer.To(d.Get("allow_gateway_transit").(bool)) | |
} | |
if d.HasChange("allow_virtual_network_access") { | |
existing.Model.Properties.AllowVirtualNetworkAccess = pointer.To(d.Get("allow_virtual_network_access").(bool)) | |
} | |
if d.HasChange("use_remote_gateways") { | |
existing.Model.Properties.UseRemoteGateways = pointer.To(d.Get("use_remote_gateways").(bool)) | |
} |
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.
Fixed.
existing, err := client.Get(ctx, id) | ||
if err != nil { | ||
if !response.WasNotFound(existing.HttpResponse) { | ||
return fmt.Errorf("checking for presence of existing Databricks %s: %s", id, err) | ||
} | ||
} | ||
|
||
if !response.WasNotFound(existing.HttpResponse) { | ||
return tf.ImportAsExistsError("azurerm_databricks_virtual_network_peering", id.ID()) | ||
} | ||
|
||
allowForwardedTraffic := d.Get("allow_forwarded_traffic").(bool) | ||
allowGatewayTransit := d.Get("allow_gateway_transit").(bool) | ||
allowVirtualNetworkAccess := d.Get("allow_virtual_network_access").(bool) | ||
useRemoteGateways := d.Get("use_remote_gateways").(bool) | ||
remoteVirtualNetwork := d.Get("remote_virtual_network_id").(string) | ||
databricksAddressSpace := utils.ExpandStringSlice(d.Get("address_space_prefixes").([]interface{})) | ||
remoteAddressSpace := utils.ExpandStringSlice(d.Get("remote_address_space_prefixes").([]interface{})) | ||
|
||
props := vnetpeering.VirtualNetworkPeeringPropertiesFormat{ | ||
DatabricksAddressSpace: &vnetpeering.AddressSpace{ | ||
AddressPrefixes: databricksAddressSpace, | ||
}, | ||
// The RP always creates the same vNet ID for the Databricks internal vNet in the below format: | ||
// '/subscriptions/{subscription}/resourceGroups/{group1}/providers/Microsoft.Network/virtualNetworks/workers-vnet' | ||
DatabricksVirtualNetwork: &vnetpeering.VirtualNetworkPeeringPropertiesFormatDatabricksVirtualNetwork{ | ||
Id: utils.String(networkParse.NewVirtualNetworkID(id.SubscriptionId, id.ResourceGroupName, "workers-vnet").ID()), | ||
}, | ||
RemoteAddressSpace: &vnetpeering.AddressSpace{ | ||
AddressPrefixes: remoteAddressSpace, | ||
}, | ||
RemoteVirtualNetwork: vnetpeering.VirtualNetworkPeeringPropertiesFormatRemoteVirtualNetwork{ | ||
Id: utils.String(remoteVirtualNetwork), | ||
}, | ||
AllowForwardedTraffic: &allowForwardedTraffic, | ||
AllowGatewayTransit: &allowGatewayTransit, | ||
AllowVirtualNetworkAccess: &allowVirtualNetworkAccess, | ||
UseRemoteGateways: &useRemoteGateways, | ||
} | ||
|
||
peer := vnetpeering.VirtualNetworkPeering{ | ||
Name: &id.VirtualNetworkPeeringName, | ||
Properties: props, | ||
} | ||
|
||
locks.ByID(databricksVnetPeeringsResourceType) | ||
defer locks.UnlockByID(databricksVnetPeeringsResourceType) |
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.
presumably the lock wants moving up to the top?
Also, per ARM we don't need to set Name btw, the API should be ignoring/rejecting it per the ARM Spec:
existing, err := client.Get(ctx, id) | |
if err != nil { | |
if !response.WasNotFound(existing.HttpResponse) { | |
return fmt.Errorf("checking for presence of existing Databricks %s: %s", id, err) | |
} | |
} | |
if !response.WasNotFound(existing.HttpResponse) { | |
return tf.ImportAsExistsError("azurerm_databricks_virtual_network_peering", id.ID()) | |
} | |
allowForwardedTraffic := d.Get("allow_forwarded_traffic").(bool) | |
allowGatewayTransit := d.Get("allow_gateway_transit").(bool) | |
allowVirtualNetworkAccess := d.Get("allow_virtual_network_access").(bool) | |
useRemoteGateways := d.Get("use_remote_gateways").(bool) | |
remoteVirtualNetwork := d.Get("remote_virtual_network_id").(string) | |
databricksAddressSpace := utils.ExpandStringSlice(d.Get("address_space_prefixes").([]interface{})) | |
remoteAddressSpace := utils.ExpandStringSlice(d.Get("remote_address_space_prefixes").([]interface{})) | |
props := vnetpeering.VirtualNetworkPeeringPropertiesFormat{ | |
DatabricksAddressSpace: &vnetpeering.AddressSpace{ | |
AddressPrefixes: databricksAddressSpace, | |
}, | |
// The RP always creates the same vNet ID for the Databricks internal vNet in the below format: | |
// '/subscriptions/{subscription}/resourceGroups/{group1}/providers/Microsoft.Network/virtualNetworks/workers-vnet' | |
DatabricksVirtualNetwork: &vnetpeering.VirtualNetworkPeeringPropertiesFormatDatabricksVirtualNetwork{ | |
Id: utils.String(networkParse.NewVirtualNetworkID(id.SubscriptionId, id.ResourceGroupName, "workers-vnet").ID()), | |
}, | |
RemoteAddressSpace: &vnetpeering.AddressSpace{ | |
AddressPrefixes: remoteAddressSpace, | |
}, | |
RemoteVirtualNetwork: vnetpeering.VirtualNetworkPeeringPropertiesFormatRemoteVirtualNetwork{ | |
Id: utils.String(remoteVirtualNetwork), | |
}, | |
AllowForwardedTraffic: &allowForwardedTraffic, | |
AllowGatewayTransit: &allowGatewayTransit, | |
AllowVirtualNetworkAccess: &allowVirtualNetworkAccess, | |
UseRemoteGateways: &useRemoteGateways, | |
} | |
peer := vnetpeering.VirtualNetworkPeering{ | |
Name: &id.VirtualNetworkPeeringName, | |
Properties: props, | |
} | |
locks.ByID(databricksVnetPeeringsResourceType) | |
defer locks.UnlockByID(databricksVnetPeeringsResourceType) | |
locks.ByID(databricksVnetPeeringsResourceType) | |
defer locks.UnlockByID(databricksVnetPeeringsResourceType) | |
existing, err := client.Get(ctx, id) | |
if err != nil { | |
if !response.WasNotFound(existing.HttpResponse) { | |
return fmt.Errorf("checking for presence of existing Databricks %s: %s", id, err) | |
} | |
} | |
if !response.WasNotFound(existing.HttpResponse) { | |
return tf.ImportAsExistsError("azurerm_databricks_virtual_network_peering", id.ID()) | |
} | |
allowForwardedTraffic := d.Get("allow_forwarded_traffic").(bool) | |
allowGatewayTransit := d.Get("allow_gateway_transit").(bool) | |
allowVirtualNetworkAccess := d.Get("allow_virtual_network_access").(bool) | |
useRemoteGateways := d.Get("use_remote_gateways").(bool) | |
remoteVirtualNetwork := d.Get("remote_virtual_network_id").(string) | |
databricksAddressSpace := utils.ExpandStringSlice(d.Get("address_space_prefixes").([]interface{})) | |
remoteAddressSpace := utils.ExpandStringSlice(d.Get("remote_address_space_prefixes").([]interface{})) | |
props := vnetpeering.VirtualNetworkPeeringPropertiesFormat{ | |
DatabricksAddressSpace: &vnetpeering.AddressSpace{ | |
AddressPrefixes: databricksAddressSpace, | |
}, | |
// The RP always creates the same vNet ID for the Databricks internal vNet in the below format: | |
// '/subscriptions/{subscription}/resourceGroups/{group1}/providers/Microsoft.Network/virtualNetworks/workers-vnet' | |
DatabricksVirtualNetwork: &vnetpeering.VirtualNetworkPeeringPropertiesFormatDatabricksVirtualNetwork{ | |
Id: utils.String(networkParse.NewVirtualNetworkID(id.SubscriptionId, id.ResourceGroupName, "workers-vnet").ID()), | |
}, | |
RemoteAddressSpace: &vnetpeering.AddressSpace{ | |
AddressPrefixes: remoteAddressSpace, | |
}, | |
RemoteVirtualNetwork: vnetpeering.VirtualNetworkPeeringPropertiesFormatRemoteVirtualNetwork{ | |
Id: utils.String(remoteVirtualNetwork), | |
}, | |
AllowForwardedTraffic: &allowForwardedTraffic, | |
AllowGatewayTransit: &allowGatewayTransit, | |
AllowVirtualNetworkAccess: &allowVirtualNetworkAccess, | |
UseRemoteGateways: &useRemoteGateways, | |
} | |
peer := vnetpeering.VirtualNetworkPeering{ | |
Properties: props, | |
} |
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.
Fixed.
deadline, ok := ctx.Deadline() | ||
if !ok { | ||
return fmt.Errorf("internal-error: context had no deadline") | ||
} | ||
|
||
stateConf := &pluginsdk.StateChangeConf{ | ||
Pending: []string{"Pending"}, | ||
Target: []string{"Created"}, | ||
Timeout: time.Until(deadline), | ||
Delay: 15 * time.Second, | ||
Refresh: func() (interface{}, string, error) { | ||
future, err := client.CreateOrUpdate(ctx, id, peer) | ||
if err != nil { | ||
if utils.ResponseErrorIsRetryable(err) { | ||
return future.HttpResponse, "Pending", err | ||
} else { | ||
if resp := future.HttpResponse; resp != nil && response.WasBadRequest(resp) && strings.Contains(err.Error(), "ReferencedResourceNotProvisioned") { | ||
// Resource is not yet ready, this may be the case if the Vnet was just created or another peering was just initiated. | ||
return future.HttpResponse, "Pending", err | ||
} | ||
} | ||
|
||
return future.HttpResponse, "", err | ||
} | ||
|
||
if err = future.Poller.PollUntilDone(ctx); err != nil { | ||
return future.HttpResponse, "", err | ||
} | ||
|
||
return future.HttpResponse, "Created", nil | ||
}, | ||
} | ||
|
||
if _, err := stateConf.WaitForStateContext(ctx); err != nil { | ||
return fmt.Errorf("waiting for Databricks %s to be created: %+v", id, 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.
since we're locking on the creation and we wait for the dependent resources to be created (since we won't return either the Virtual Network or the DataBricks Cluster until it's marked as fully provisioned) - I don't believe this is still needed - so I'm wondering if we can switch to using the regular LRO directly and remove the WaitForState
func entirely?
deadline, ok := ctx.Deadline() | |
if !ok { | |
return fmt.Errorf("internal-error: context had no deadline") | |
} | |
stateConf := &pluginsdk.StateChangeConf{ | |
Pending: []string{"Pending"}, | |
Target: []string{"Created"}, | |
Timeout: time.Until(deadline), | |
Delay: 15 * time.Second, | |
Refresh: func() (interface{}, string, error) { | |
future, err := client.CreateOrUpdate(ctx, id, peer) | |
if err != nil { | |
if utils.ResponseErrorIsRetryable(err) { | |
return future.HttpResponse, "Pending", err | |
} else { | |
if resp := future.HttpResponse; resp != nil && response.WasBadRequest(resp) && strings.Contains(err.Error(), "ReferencedResourceNotProvisioned") { | |
// Resource is not yet ready, this may be the case if the Vnet was just created or another peering was just initiated. | |
return future.HttpResponse, "Pending", err | |
} | |
} | |
return future.HttpResponse, "", err | |
} | |
if err = future.Poller.PollUntilDone(ctx); err != nil { | |
return future.HttpResponse, "", err | |
} | |
return future.HttpResponse, "Created", nil | |
}, | |
} | |
if _, err := stateConf.WaitForStateContext(ctx); err != nil { | |
return fmt.Errorf("waiting for Databricks %s to be created: %+v", id, err) | |
} | |
if err := client.CreateOrUpdateThenPoll(ctx, id, peer); err != nil { | |
return fmt.Errorf("creating Databricks %s: %+v", id, 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.
Fixed.
future, err := client.CreateOrUpdate(ctx, *id, *existing.Model) | ||
if err != nil { | ||
return fmt.Errorf("updating Databricks %s: %+v", *id, err) | ||
} | ||
|
||
if err := future.Poller.PollUntilDone(ctx); err != nil { | ||
return fmt.Errorf("waiting for update of Databricks %s: %+v", *id, 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.
minor but FYI this can be reduced too:
future, err := client.CreateOrUpdate(ctx, *id, *existing.Model) | |
if err != nil { | |
return fmt.Errorf("updating Databricks %s: %+v", *id, err) | |
} | |
if err := future.Poller.PollUntilDone(ctx); err != nil { | |
return fmt.Errorf("waiting for update of Databricks %s: %+v", *id, err) | |
} | |
if err := client.CreateOrUpdateThenPoll(ctx, *id, *existing.Model); err != nil { | |
return fmt.Errorf("updating Databricks %s: %+v", *id, 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.
Fixed.
|
||
resp, err := clients.DataBricks.VnetPeeringClient.Get(ctx, *id) | ||
if err != nil { | ||
return nil, fmt.Errorf("making Read request on Databricks %s: %+v", *id, 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.
since the Exists function is used for both checking it exists and that it doesn't, we'll want to handle the 404 here when it doesn't, as that's raised as an error (since we're not expecting a 404 by default when calling the Get
function):
return nil, fmt.Errorf("making Read request on Databricks %s: %+v", *id, err) | |
if response.WasNotFound(resp.HttpResponse) { | |
return pointer.To(false), nil | |
} | |
return nil, fmt.Errorf("making Read request on Databricks %s: %+v", *id, 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.
Fixed.
@@ -0,0 +1,4 @@ | |||
package databricks |
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 file shouldn't be needed anymore, since it's available within go-azure-sdk
?
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.
Fixed.
@@ -0,0 +1,23 @@ | |||
package validate |
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 file shouldn't be needed anymore, since it's available within go-azure-sdk
?
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.
Fixed.
@@ -0,0 +1,88 @@ | |||
package validate |
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 file shouldn't be needed anymore, since it's available within go-azure-sdk
?
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.
Fixed.
@@ -0,0 +1,23 @@ | |||
package validate |
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 file shouldn't be needed anymore, since it's available within go-azure-sdk
?
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.
Fixed.
@@ -0,0 +1,76 @@ | |||
package validate |
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 file shouldn't be needed anymore, since it's available within go-azure-sdk
?
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.
Fixed.
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.
LGTM 🧊
This functionality has been released in v3.49.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
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. |
Adding new resource
azurerm_databricks_virtual_network_peering
to support functionality which GA'ed on2023-01-31