Skip to content
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

Merged
merged 25 commits into from
Mar 24, 2023
Merged
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
b8e3272
Update go-azure-sdk v0.20230228.1160358 => v0.20230301.1141943
WodansSon Mar 2, 2023
54c4add
Check-in progress...
WodansSon Mar 2, 2023
9bfdd46
Check-in progress...
WodansSon Mar 2, 2023
37dd29f
pull changes from main
WodansSon Mar 2, 2023
7dd41e2
Check-in progress...
WodansSon Mar 2, 2023
d25ed01
Checking-in progress...
WodansSon Mar 4, 2023
a7edac1
terrafmt docs...
WodansSon Mar 4, 2023
cffd531
update test case to use sku parameter...
WodansSon Mar 4, 2023
7ddaec2
Check-in progress...
WodansSon Mar 4, 2023
99fff59
Remove test case...
WodansSon Mar 4, 2023
05838f0
Check-in progress...
WodansSon Mar 4, 2023
c2cef32
match field names with network resource
WodansSon Mar 6, 2023
4d9c7e4
add comment about delete SDK issue...
WodansSon Mar 7, 2023
93d5d18
Per PR review moved resource mutex from sync to the terraform interna…
WodansSon Mar 15, 2023
361abde
Checking-in PR changes progress...
WodansSon Mar 18, 2023
51dd98a
Merge branch 'main' of https://github.com/hashicorp/terraform-provide…
WodansSon Mar 18, 2023
b833626
go mod vendor
WodansSon Mar 18, 2023
42dab33
Update client name to match SDK...
WodansSon Mar 18, 2023
ad7868d
Check-in progress...
WodansSon Mar 20, 2023
4073635
Remove workaround for SDK issue...
WodansSon Mar 20, 2023
390cbf1
Update delete func to be just DeleteThenPoll...
WodansSon Mar 22, 2023
ff03612
Address additional PR comments...
WodansSon Mar 22, 2023
d5a2ee6
Merge branch 'main' of https://github.com/hashicorp/terraform-provide…
WodansSon Mar 22, 2023
31efc4d
Add test workaround for AzSecPack...
WodansSon Mar 22, 2023
c42e391
Remove parse directory...
WodansSon Mar 22, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Per PR review moved resource mutex from sync to the terraform interna…
…l locks package...
  • Loading branch information
WodansSon committed Mar 15, 2023
commit 93d5d18abc519a36a189688007a7ce4366f14fbc
Original file line number Diff line number Diff line change
@@ -4,14 +4,14 @@ import (
"fmt"
"log"
"strings"
"sync"
"time"

"github.com/hashicorp/go-azure-helpers/lang/response"
"github.com/hashicorp/go-azure-helpers/resourcemanager/commonschema"
"github.com/hashicorp/go-azure-sdk/resource-manager/databricks/2023-02-01/vnetpeering"
"github.com/hashicorp/terraform-provider-azurerm/helpers/tf"
"github.com/hashicorp/terraform-provider-azurerm/internal/clients"
"github.com/hashicorp/terraform-provider-azurerm/internal/locks"
"github.com/hashicorp/terraform-provider-azurerm/internal/services/databricks/validate"
networkValidate "github.com/hashicorp/terraform-provider-azurerm/internal/services/network/validate"
"github.com/hashicorp/terraform-provider-azurerm/internal/tf/pluginsdk"
@@ -21,7 +21,9 @@ import (

// peerMutex is used to prevent multiple Peering resources being created, updated
// or deleted at the same time
var peerMutex = &sync.Mutex{}
const resourceLock string = "databricks.VnetPeerings"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

every other lock in the provider uses the resource type here, can we update this to match:

Suggested change
const resourceLock string = "databricks.VnetPeerings"
const databricksVnetPeeringsResourceType string = "azurerm_databricks_virtual_network_peering"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


var peerMutex = locks.NewMutexKV()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can use locks.ByName(id.Name, databricksVnetPeeringsResourceType) here instead, we shouldn't be newing up a new lock here:

Suggested change
var peerMutex = locks.NewMutexKV()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


func resourceDatabricksVirtualNetworkPeering() *pluginsdk.Resource {
return &pluginsdk.Resource{
@@ -173,8 +175,8 @@ func resourceDatabricksVirtualNetworkPeeringCreateUpdate(d *pluginsdk.ResourceDa
Id: utils.String(virtualNetworkId),
}

peerMutex.Lock()
defer peerMutex.Unlock()
peerMutex.Lock(resourceLock)
defer peerMutex.Unlock(resourceLock)
Copy link
Contributor

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


if err := pluginsdk.Retry(300*time.Second, retryDatabricksVnetPeeringsClientCreateUpdate(d, id, peer, meta)); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

  1. Is this logic needed? The CreateOrUpdate method is a Future, so it should be handling this in the API?
  2. Whilst I appreciate this is a copy of the existing azurerm_virtual_network_peering resource (which is, unbeknown to me still using a hard-coded timeout) - we shouldn't be using a hard-coded timeout like this, instead we should use the timeout from the context to allow users to override this.

If this is still needed, can we instead update this to use a WaitForState func:

Suggested change
if err := pluginsdk.Retry(300*time.Second, retryDatabricksVnetPeeringsClientCreateUpdate(d, id, peer, meta)); err != nil {
deadline, ok := ctx.Deadline()
if !ok {
return fmt.Errorf("internal-error: context had no deadline")
}
stateConf := &pluginsdk.StateChangeConf{
Pending: []string{},
Target: []string{"Created"},
Refresh: func() (interface{}, string, error) {
resp, err := client.CreateOrUpdate(ctx, *id, peer)
if err != nil {
return resp, "Failed" fmt.Errorf("deleting %s: %+v", id, err)
}
if err := resp.Poller.PollUntilDone(ctx); err != nil {
return resp, "Failed" fmt.Errorf("waiting for the creation of %s: %+v", id, err)
}
return res, "Created", nil
},
MinTimeout: 15 * time.Second,
Timeout: time.Until(deadline)
}
if _, err := stateConf.WaitForStateContext(ctx); err != nil {
return fmt.Errorf("creating %s: %+v", id, err)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

return err
@@ -258,8 +260,8 @@ func resourceDatabricksVirtualNetworkPeeringDelete(d *pluginsdk.ResourceData, me
return nil
}

peerMutex.Lock()
defer peerMutex.Unlock()
peerMutex.Lock(resourceLock)
defer peerMutex.Unlock(resourceLock)

// this is a workaround for the SDK issue: https://github.com/hashicorp/go-azure-sdk/issues/351
// and will be cleaned up once the fix has been implemented.