-
Notifications
You must be signed in to change notification settings - Fork 5k
azurerm_servicebus_namespace - split create update funcs
#28539
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
Changes from 3 commits
1b3f2c1
fa7a405
49e8907
750b17e
06ca75c
2a7b17e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -32,7 +32,6 @@ import ( | |||||||||||||||||||||
| "github.com/hashicorp/terraform-provider-azurerm/internal/tf/suppress" | ||||||||||||||||||||||
| "github.com/hashicorp/terraform-provider-azurerm/internal/tf/validation" | ||||||||||||||||||||||
| "github.com/hashicorp/terraform-provider-azurerm/internal/timeouts" | ||||||||||||||||||||||
| "github.com/hashicorp/terraform-provider-azurerm/utils" | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Default Authorization Rule/Policy created by Azure, used to populate the | ||||||||||||||||||||||
|
|
@@ -44,9 +43,9 @@ var ( | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| func resourceServiceBusNamespace() *pluginsdk.Resource { | ||||||||||||||||||||||
| resource := &pluginsdk.Resource{ | ||||||||||||||||||||||
| Create: resourceServiceBusNamespaceCreateUpdate, | ||||||||||||||||||||||
| Create: resourceServiceBusNamespaceCreate, | ||||||||||||||||||||||
| Read: resourceServiceBusNamespaceRead, | ||||||||||||||||||||||
| Update: resourceServiceBusNamespaceCreateUpdate, | ||||||||||||||||||||||
| Update: resourceServiceBusNamespaceUpdate, | ||||||||||||||||||||||
| Delete: resourceServiceBusNamespaceDelete, | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Importer: pluginsdk.ImporterValidatingResourceId(func(id string) error { | ||||||||||||||||||||||
|
|
@@ -119,7 +118,7 @@ func resourceServiceBusNamespace() *pluginsdk.Resource { | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| "identity_id": { | ||||||||||||||||||||||
| Type: pluginsdk.TypeString, | ||||||||||||||||||||||
| Required: true, | ||||||||||||||||||||||
| Optional: true, | ||||||||||||||||||||||
| ValidateFunc: commonids.ValidateUserAssignedIdentityID, | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -272,13 +271,13 @@ func resourceServiceBusNamespace() *pluginsdk.Resource { | |||||||||||||||||||||
| return resource | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| func resourceServiceBusNamespaceCreateUpdate(d *pluginsdk.ResourceData, meta interface{}) error { | ||||||||||||||||||||||
| func resourceServiceBusNamespaceCreate(d *pluginsdk.ResourceData, meta interface{}) error { | ||||||||||||||||||||||
| client := meta.(*clients.Client).ServiceBus.NamespacesClient | ||||||||||||||||||||||
| subscriptionId := meta.(*clients.Client).Account.SubscriptionId | ||||||||||||||||||||||
| ctx, cancel := timeouts.ForCreateUpdate(meta.(*clients.Client).StopContext, d) | ||||||||||||||||||||||
| ctx, cancel := timeouts.ForCreate(meta.(*clients.Client).StopContext, d) | ||||||||||||||||||||||
| defer cancel() | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| log.Printf("[INFO] preparing arguments for ServiceBus Namespace create/update.") | ||||||||||||||||||||||
| log.Printf("[INFO] preparing arguments for ServiceBus Namespace create") | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| location := azure.NormalizeLocation(d.Get("location").(string)) | ||||||||||||||||||||||
| sku := d.Get("sku").(string) | ||||||||||||||||||||||
|
|
@@ -318,7 +317,7 @@ func resourceServiceBusNamespaceCreateUpdate(d *pluginsdk.ResourceData, meta int | |||||||||||||||||||||
| }, | ||||||||||||||||||||||
| Properties: &namespaces.SBNamespaceProperties{ | ||||||||||||||||||||||
| Encryption: expandServiceBusNamespaceEncryption(d.Get("customer_managed_key").([]interface{})), | ||||||||||||||||||||||
| DisableLocalAuth: utils.Bool(!d.Get("local_auth_enabled").(bool)), | ||||||||||||||||||||||
| DisableLocalAuth: pointer.To(!d.Get("local_auth_enabled").(bool)), | ||||||||||||||||||||||
| PublicNetworkAccess: &publicNetworkEnabled, | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| Tags: expandTags(t), | ||||||||||||||||||||||
|
|
@@ -336,7 +335,7 @@ func resourceServiceBusNamespaceCreateUpdate(d *pluginsdk.ResourceData, meta int | |||||||||||||||||||||
| if strings.EqualFold(sku, string(namespaces.SkuNamePremium)) && capacity.(int) == 0 { | ||||||||||||||||||||||
| return fmt.Errorf("service bus SKU %q only supports `capacity` of 1, 2, 4, 8 or 16", sku) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| parameters.Sku.Capacity = utils.Int64(int64(capacity.(int))) | ||||||||||||||||||||||
| parameters.Sku.Capacity = pointer.To(int64(capacity.(int))) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if premiumMessagingUnit := d.Get("premium_messaging_partitions"); premiumMessagingUnit != nil { | ||||||||||||||||||||||
|
|
@@ -346,11 +345,113 @@ func resourceServiceBusNamespaceCreateUpdate(d *pluginsdk.ResourceData, meta int | |||||||||||||||||||||
| if strings.EqualFold(sku, string(namespaces.SkuNamePremium)) && premiumMessagingUnit.(int) == 0 { | ||||||||||||||||||||||
| return fmt.Errorf("service bus SKU %q only supports `premium_messaging_partitions` of 1, 2, 4", sku) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| parameters.Properties.PremiumMessagingPartitions = utils.Int64(int64(premiumMessagingUnit.(int))) | ||||||||||||||||||||||
| parameters.Properties.PremiumMessagingPartitions = pointer.To(int64(premiumMessagingUnit.(int))) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if err := client.CreateOrUpdateThenPoll(ctx, id, parameters); err != nil { | ||||||||||||||||||||||
| return fmt.Errorf("creating/updating %s: %+v", id, err) | ||||||||||||||||||||||
| return fmt.Errorf("creating %s: %+v", id, err) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| d.SetId(id.ID()) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if d.HasChange("network_rule_set") { | ||||||||||||||||||||||
|
||||||||||||||||||||||
| networkRuleSet := d.Get("network_rule_set").([]interface{}) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| log.Printf("[DEBUG] Creating the Network Rule Set associated with %s..", id) | ||||||||||||||||||||||
| if err = createNetworkRuleSetForNamespace(ctx, client, id, networkRuleSet); err != nil { | ||||||||||||||||||||||
| return err | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| log.Printf("[DEBUG] Created the Network Rule Set associated with %s", id) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return resourceServiceBusNamespaceRead(d, meta) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| func resourceServiceBusNamespaceUpdate(d *pluginsdk.ResourceData, meta interface{}) error { | ||||||||||||||||||||||
| client := meta.(*clients.Client).ServiceBus.NamespacesClient | ||||||||||||||||||||||
| ctx, cancel := timeouts.ForUpdate(meta.(*clients.Client).StopContext, d) | ||||||||||||||||||||||
| defer cancel() | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| log.Printf("[INFO] preparing arguments for ServiceBus Namespace update") | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| id, err := namespaces.ParseNamespaceID(d.Id()) | ||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||
| return err | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| existing, err := client.Get(ctx, *id) | ||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||
| return fmt.Errorf("retrieving %s: %+v", *id, err) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if existing.Model == nil { | ||||||||||||||||||||||
| return fmt.Errorf("retrieving existing %s: `model` was nil", *id) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| if existing.Model.Properties == nil { | ||||||||||||||||||||||
| return fmt.Errorf("retrieving existing %s: `model.Properties` was nil", *id) | ||||||||||||||||||||||
|
||||||||||||||||||||||
| if existing.Model == nil { | |
| return fmt.Errorf("retrieving existing %s: `model` was nil", *id) | |
| } | |
| if existing.Model.Properties == nil { | |
| return fmt.Errorf("retrieving existing %s: `model.Properties` was nil", *id) | |
| if existing.Model == nil { | |
| return fmt.Errorf("retrieving %s: `model` was nil", *id) | |
| } | |
| if existing.Model.Properties == nil { | |
| return fmt.Errorf("retrieving %s: `model.Properties` was nil", *id) |
Outdated
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.
| minimumTls := namespaces.TlsVersion(tlsValue) | |
| payload.Properties.MinimumTlsVersion = &minimumTls | |
| payload.Properties.MinimumTlsVersion = pointer.To(namespaces.TlsVersion(tlsValue)) |
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.
I suspect there's a typo here and one of these conditions should be comparing against a different sku
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.
I didn't write this, just copied from the create, but I believe it is correct. The first check is that it is NOT premium and greater than 0, because basic and standard skus can only have a capacity of 0. The second check returns an error if it is premium and set to 0 because if the sku is premium the sku needs to be 1,2,4,8 or 16.
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.
You're right, I missed the not at the front of the first condition.
Outdated
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.
Should this just be
| log.Printf("[DEBUG] Creating/updating the Network Rule Set associated with %s..", id) | |
| log.Printf("[DEBUG] Updating the Network Rule Set associated with %s..", id) |
Outdated
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.
Likewise
| log.Printf("[DEBUG] Created/updated the Network Rule Set associated with %s", id) | |
| log.Printf("[DEBUG] Updated the Network Rule Set associated with %s", id) |
Outdated
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 we allow identity_id to be Optional, can users actually create a servicebus namespace with CMK using SystemAssigned identity on first 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.
they can't, so I've reverted this to how it was before 👍
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -87,7 +87,7 @@ A `customer_managed_key` block supports the following: | |||||
|
|
||||||
| * `key_vault_key_id` - (Required) The ID of the Key Vault Key which should be used to Encrypt the data in this ServiceBus Namespace. | ||||||
|
|
||||||
| * `identity_id` - (Required) The ID of the User Assigned Identity that has access to the key. | ||||||
| * `identity_id` - (Optional) The ID of the User Assigned Identity that has access to the key. | ||||||
|
||||||
| * `identity_id` - (Optional) The ID of the User Assigned Identity that has access to the key. | |
| * `identity_id` - (Required) The ID of the User Assigned Identity that has access to the key. |
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! thank you
Uh oh!
There was an error while loading. Please reload this page.
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.
I can't comment further down, but I don't think we need the
d.IsNewResource()check down on line 287 anymore