azurerm_servicebus_namespace - split create update funcs#28539
azurerm_servicebus_namespace - split create update funcs#28539catriona-m merged 6 commits intomainfrom
azurerm_servicebus_namespace - split create update funcs#28539Conversation
| log.Printf("[INFO] preparing arguments for ServiceBus Namespace create") | ||
|
|
||
| location := azure.NormalizeLocation(d.Get("location").(string)) | ||
| sku := d.Get("sku").(string) |
There was a problem hiding this comment.
I can't comment further down, but I don't think we need the d.IsNewResource() check down on line 287 anymore
|
|
||
| d.SetId(id.ID()) | ||
|
|
||
| if d.HasChange("network_rule_set") { |
There was a problem hiding this comment.
Without any context here, why do we need to wrap this update on the network rule set ind.HasChange in the Create?
| 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) |
There was a problem hiding this comment.
| 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) |
| minimumTls := namespaces.TlsVersion(tlsValue) | ||
| payload.Properties.MinimumTlsVersion = &minimumTls |
There was a problem hiding this comment.
| minimumTls := namespaces.TlsVersion(tlsValue) | |
| payload.Properties.MinimumTlsVersion = &minimumTls | |
| payload.Properties.MinimumTlsVersion = pointer.To(namespaces.TlsVersion(tlsValue)) |
| } else { | ||
| log.Printf("[DEBUG] Creating the Network Rule Set associated with %s..", id) | ||
| if err = createNetworkRuleSetForNamespace(ctx, client, id, newNetworkRuleSet.([]interface{})); err != nil { | ||
| log.Printf("[DEBUG] Creating/updating the Network Rule Set associated with %s..", id) |
There was a problem hiding this comment.
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) |
| return err | ||
| } | ||
| log.Printf("[DEBUG] Created the Network Rule Set associated with %s", id) | ||
| log.Printf("[DEBUG] Created/updated the Network Rule Set associated with %s", id) |
There was a problem hiding this comment.
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) |
| if v["identity_id"].(string) == "" { | ||
| encryption.KeyVaultProperties = &[]namespaces.KeyVaultProperties{ | ||
| { | ||
| KeyName: pointer.To(keyId.Name), | ||
| KeyVersion: pointer.To(keyId.Version), | ||
| KeyVaultUri: pointer.To(keyId.KeyVaultBaseUrl), | ||
| }, | ||
| } |
There was a problem hiding this comment.
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.
they can't, so I've reverted this to how it was before 👍
| if !strings.EqualFold(sku, string(namespaces.SkuNamePremium)) && capacity.(int) > 0 { | ||
| return fmt.Errorf("service bus SKU %q only supports `capacity` of 0", sku) | ||
| } | ||
| if strings.EqualFold(sku, string(namespaces.SkuNamePremium)) && capacity.(int) == 0 { |
There was a problem hiding this comment.
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.
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.
You're right, I missed the not at the front of the first condition.
| * `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. |
There was a problem hiding this comment.
| * `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. |
* Update CHANGELOG.md for #28840 * Update CHANGELOG.md #28808 * Update CHANGELOG.md #27962 * Update CHANGELOG.md for #28859 * Update for #28825 * Update CHANGELOG.md for #28864 * Update CHANGELOG.md #28539 * Update CHANGELOG.md #28685 * Update CHANGELOG.md for #28818 * Update for #28857 #28799 #28856 * Update for #28122 * Update for #28248 #27805 * Update for #28853 * Update for #28316 #28494 #28696 * Update for #28754 * Update CHANGELOG.md #28771 * Update CHANGELOG.md #28842 * Update for #28879 * Update for #28199 * Update CHANGELOG.md #28862 * prep for release v4.21.0 --------- Co-authored-by: sreallymatt <106555974+sreallymatt@users.noreply.github.com> Co-authored-by: Wodans Son <20408400+WodansSon@users.noreply.github.com> Co-authored-by: stephybun <steph@hashicorp.com> Co-authored-by: catriona-m <86247157+catriona-m@users.noreply.github.com> Co-authored-by: Matthew Frahry <mbfrahry@gmail.com> Co-authored-by: Wyatt Fry <wyattfry@gmail.com>
|
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. |
2 participants
Community Note
Description
This PR splits the create and update functions so that ignore_changes works as expected.
PR Checklist
For example: “
resource_name_here- description of change e.g. adding propertynew_property_name_here”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_servicebus_namespace- split create/update functions to enable use ofignore_changes[GH-00000]This is a (please select all that apply):
Related Issue(s)
Fixes #0000
Note
If this PR changes meaningfully during the course of review please update the title and description as required.