-
Notifications
You must be signed in to change notification settings - Fork 102
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
Correct README examples and add enhancement #26
Conversation
README.md
Outdated
lb_port = { | ||
http = ["80", "Tcp", "80"] | ||
https = ["443", "Tcp", "443"] | ||
https2 = ["1443", "Tcp", "1443", "/", "Https"] |
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.
In variables.tf, the definition of "lb_port" is "...[frontend_port, protocol, backend_port]...", i.e. there are totally 3 parameters, while here the "https2" has 5 parameters. Is it a mismatch or any description needs to be changed in the variables.tf?
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 have changed the corresponding description in variables.tf
main.tf
Outdated
loadbalancer_id = azurerm_lb.azlb.id | ||
protocol = element(var.lb_port[element(keys(var.lb_port), count.index)], 1) | ||
port = element(var.lb_port[element(keys(var.lb_port), count.index)], 2) | ||
interval_in_seconds = var.lb_probe_interval | ||
number_of_probes = var.lb_probe_unhealthy_threshold | ||
request_path = lower(element(var.lb_port[element(keys(var.lb_port), count.index)], 3)) == "http" || lower(element(var.lb_port[element(keys(var.lb_port), count.index)], 3)) == "https" ? element(var.lb_port[element(keys(var.lb_port), count.index)], 4) : "" |
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.
In the Azure LB TF doc, the "request_path" refers to a URI. Seems the assigned value to request_path is not a URI type. Or did I misunderstand anything?
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.
Yes. It's defined as a URI. But there is no validation restriction in lb_probe_resource
and in service api. I have tested the example works fine. I assume \
is acceptable for the service.
test/fixture/main.tf
Outdated
http = ["80", "Tcp", "80"] | ||
http = ["80", "Tcp", "80"] | ||
https = ["443", "Tcp", "443"] | ||
https2 = ["1443", "Tcp", "1443", "/", "Https"] |
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.
The same question as above about the mis-match between 3 parameters mentioned in doc and 5 parameters here
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.
On fact, I have changed the description in variables.tf.
@yupwei68 , thank you for making this PR. I have some questions and appreciate if you could help me understand. thanks. |
Hi @mybayern1974 Thanks for your comments. I have replied in the comments, please continue reviewing. |
Hi @mybayern1974 I found that the protocol type of |
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.
LGTM
Fix :#24
#18
merge PR: #14