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.

Sign up for GitHub

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

Jump to bottom

azurerm_lb: Add support for SkuTier property #13680

Merged
merged 4 commits into from
Oct 14, 2021
Merged

azurerm_lb: Add support for SkuTier property #13680

merged 4 commits into from
Oct 14, 2021

Conversation

sinbai
Copy link
Contributor

@sinbai sinbai commented Oct 11, 2021

TF_ACC=1 go test ./internal/services/loadbalancer/ -v -parallel 11 -test.run=TestAccAzureRMLoadBalancer_ -timeout 1440m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN TestAccAzureRMLoadBalancer_basic
=== PAUSE TestAccAzureRMLoadBalancer_basic
=== RUN TestAccAzureRMLoadBalancer_requiresImport
=== PAUSE TestAccAzureRMLoadBalancer_requiresImport
=== RUN TestAccAzureRMLoadBalancer_standard
=== PAUSE TestAccAzureRMLoadBalancer_standard
=== RUN TestAccAzureRMLoadBalancer_frontEndConfigPublicIPPrefix
=== PAUSE TestAccAzureRMLoadBalancer_frontEndConfigPublicIPPrefix
=== RUN TestAccAzureRMLoadBalancer_frontEndConfig
=== PAUSE TestAccAzureRMLoadBalancer_frontEndConfig
=== RUN TestAccAzureRMLoadBalancer_tags
=== PAUSE TestAccAzureRMLoadBalancer_tags
=== RUN TestAccAzureRMLoadBalancer_emptyPrivateIP
=== PAUSE TestAccAzureRMLoadBalancer_emptyPrivateIP
=== RUN TestAccAzureRMLoadBalancer_privateIP
=== PAUSE TestAccAzureRMLoadBalancer_privateIP
=== RUN TestAccAzureRMLoadBalancer_updateFrontEndConfigsWithZone
=== PAUSE TestAccAzureRMLoadBalancer_updateFrontEndConfigsWithZone
=== RUN TestAccAzureRMLoadBalancer_ZoneRedundant
=== PAUSE TestAccAzureRMLoadBalancer_ZoneRedundant
=== RUN TestAccAzureRMLoadBalancer_NoZone
=== PAUSE TestAccAzureRMLoadBalancer_NoZone
=== RUN TestAccAzureRMLoadBalancer_SingleZone
=== PAUSE TestAccAzureRMLoadBalancer_SingleZone
=== CONT TestAccAzureRMLoadBalancer_basic
=== CONT TestAccAzureRMLoadBalancer_emptyPrivateIP
=== CONT TestAccAzureRMLoadBalancer_NoZone
=== CONT TestAccAzureRMLoadBalancer_frontEndConfigPublicIPPrefix
=== CONT TestAccAzureRMLoadBalancer_tags
=== CONT TestAccAzureRMLoadBalancer_frontEndConfig
=== CONT TestAccAzureRMLoadBalancer_ZoneRedundant
=== CONT TestAccAzureRMLoadBalancer_SingleZone
=== CONT TestAccAzureRMLoadBalancer_updateFrontEndConfigsWithZone
=== CONT TestAccAzureRMLoadBalancer_standard
=== CONT TestAccAzureRMLoadBalancer_privateIP
--- PASS: TestAccAzureRMLoadBalancer_standard (224.46s)
=== CONT TestAccAzureRMLoadBalancer_requiresImport
--- PASS: TestAccAzureRMLoadBalancer_privateIP (250.09s)
--- PASS: TestAccAzureRMLoadBalancer_basic (262.77s)
--- PASS: TestAccAzureRMLoadBalancer_frontEndConfigPublicIPPrefix (273.29s)
--- PASS: TestAccAzureRMLoadBalancer_NoZone (297.52s)
--- PASS: TestAccAzureRMLoadBalancer_emptyPrivateIP (305.44s)
--- PASS: TestAccAzureRMLoadBalancer_SingleZone (306.20s)
--- PASS: TestAccAzureRMLoadBalancer_tags (320.31s)
--- PASS: TestAccAzureRMLoadBalancer_ZoneRedundant (338.00s)
--- PASS: TestAccAzureRMLoadBalancer_requiresImport (204.07s)
--- PASS: TestAccAzureRMLoadBalancer_frontEndConfig (433.65s)
--- PASS: TestAccAzureRMLoadBalancer_updateFrontEndConfigsWithZone (493.60s)
PASS
ok github.com/hashicorp/terraform-provider-azurerm/internal/services/loadbalancer 494.001s

tombuildsstuff
tombuildsstuff suggested changes Oct 11, 2021
View reviewed changes
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hi @sinbai

Thanks for this PR.

Taking a look through on the whole this looks good to me, but I've left a few comments inline - if we can fix those up then we should be able to take another look 👍

Thanks!

internal/services/loadbalancer/loadbalancer_resource.go Outdated
Comment on lines 74 to 75
}, true),
DiffSuppressFunc: suppress.CaseDifference,
Copy link
Contributor

Choose a reason for hiding this comment

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

this indicates a bug which needs to be fixed (either by normalisation on our side, or by the API returning this correctly in the first place) - so can we remove this:

Suggested change
}, true),
DiffSuppressFunc: suppress.CaseDifference,
}, false),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

internal/services/loadbalancer/loadbalancer_resource.go Outdated
@@ -252,9 +264,16 @@ func resourceArmLoadBalancerCreateUpdate(d *pluginsdk.ResourceData, meta interfa
}
}

if strings.EqualFold(d.Get("sku_tier").(string), string(network.LoadBalancerSkuTierGlobal)) {
if !strings.EqualFold(d.Get("sku").(string), string(network.LoadBalancerSkuNameStandard)) {
return fmt.Errorf("global load balancing is only supported for standard SKU load balancers %q (Resource Group %q)", id.Name, id.ResourceGroup)
Copy link
Contributor

Choose a reason for hiding this comment

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

adding the name and resource group to this error doesn't give us much?

Suggested change
return fmt.Errorf("global load balancing is only supported for standard SKU load balancers %q (Resource Group %q)", id.Name, id.ResourceGroup)
return fmt.Errorf("global load balancing is only supported for standard SKU load balancers")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

internal/services/loadbalancer/loadbalancer_resource.go
@@ -319,6 +338,7 @@ func resourceArmLoadBalancerRead(d *pluginsdk.ResourceData, meta interface{}) er

if sku := resp.Sku; sku != nil {
d.Set("sku", string(sku.Name))
d.Set("sku_tier", string(sku.Tier))
Copy link
Contributor

Choose a reason for hiding this comment

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

(as above) this should be normalised if the API's returning this in a different case to defined in the Swagger spec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The API returns the same case as defined in the Swagger spec.

website/docs/r/lb.html.markdown Outdated
@@ -46,7 +46,7 @@ The following arguments are supported:
* `location` - (Required) Specifies the supported Azure Region where the Load Balancer should be created.
* `frontend_ip_configuration` - (Optional) One or multiple `frontend_ip_configuration` blocks as documented below.
* `sku` - (Optional) The SKU of the Azure Load Balancer. Accepted values are `Basic` and `Standard`. Defaults to `Basic`.

* `sku_tier` - (Optional) The Tier of the Azure Load Balancer SKU. Accepted values are `Global` and `Regional`. Defaults to `Regional`. Changing this forces a new resource to be created.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* `sku_tier` - (Optional) The Tier of the Azure Load Balancer SKU. Accepted values are `Global` and `Regional`. Defaults to `Regional`. Changing this forces a new resource to be created.
* `sku_tier` - (Optional) The Sku Tier of this Load Balancer. Possible values are `Global` and `Regional`. Defaults to `Regional`. Changing this forces a new resource to be created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

internal/services/loadbalancer/loadbalancer_resource.go
@@ -63,6 +63,18 @@ func resourceArmLoadBalancer() *pluginsdk.Resource {
DiffSuppressFunc: suppress.CaseDifference,
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add a TODO to fix the case-sensitivity here in 3.0 too, fwiw - but it'd be good to apply the same normalization fix in the Read for this ahead of time, should the API be returning this incorrectly here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

katbyte
katbyte approved these changes Oct 14, 2021
View reviewed changes
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @sinbai - LGTM 🌿

@katbyte katbyte added this to the v2.81.0 milestone Oct 14, 2021
@katbyte katbyte merged commit cbc6d69 into hashicorp:main Oct 14, 2021
katbyte added a commit that referenced this pull request Oct 14, 2021
@katbyte
CHANGELOG.md for #13680
ba9e199
@github-actions
Copy link

This functionality has been released in v2.81.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!

@sinbai sinbai deleted the loadbalancer/sku_tier branch October 22, 2021 00:33
@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tombuildsstuff tombuildsstuff tombuildsstuff requested changes

@katbyte katbyte katbyte approved these changes

Assignees
No one assigned
Labels
documentation enhancement service/load-balancers size/XS
Projects
None yet
Milestone
v2.81.0
Development

Successfully merging this pull request may close these issues.
3 participants
@sinbai @tombuildsstuff @katbyte