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

azurerm_lb_probe: backport azurestack review comments #1742

Merged
merged 2 commits into from
Aug 9, 2018

Conversation

katbyte
Copy link
Collaborator

@katbyte katbyte commented Aug 8, 2018

backport of issues found in this pr

also explicitly adds Https support to protocol property.

@katbyte katbyte added this to the 1.13.0 milestone Aug 8, 2018
@katbyte katbyte requested a review from a team August 8, 2018 22:46
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

LGTM with some minor nits and assuming acceptance testing passes. 🚀

}
d.Set("load_balancer_rules", load_balancer_rules)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: This is a trivial case (currently) but best practice when using d.Set() with aggregate types (TypeList, TypeSet, TypeMap), we should perform error checking to prevent issues where the code is not properly able to set the Terraform state. e.g.

if err := d.Set("load_balancer_rules", load_balancer_rules); err != nil {
  return fmt.Errorf("error setting load_balancer_rules: %s", err)
}

DiffSuppressFunc: suppress.CaseDifference,
ValidateFunc: validation.StringInSlice([]string{
string(network.ProbeProtocolHTTP),
string(network.ProbeProtocolHTTPS),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: We may want to consider adding an acceptance test to cover this new value

@katbyte
Copy link
Collaborator Author

katbyte commented Aug 9, 2018

Tests pass:
screen shot 2018-08-09 at 12 16 48

@katbyte katbyte merged commit 53e1301 into master Aug 9, 2018
katbyte added a commit that referenced this pull request Aug 9, 2018
@katbyte katbyte deleted the lb-probe-azurestack_updates branch August 9, 2018 19:21
@ghost
Copy link

ghost commented Mar 30, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants