Adding the Match parameter to the Probes to check on the response code #1446
Conversation
There was a problem hiding this comment.
Hey @VaijanathB
Thanks for this PR. As you've mentioned - the Application Gateway tests are unreliable due to the broken API, as such we need to be careful with this (we've been intentionally not changing Application Gateway's until the API is fixed as the broken tests leaves resources lingering in our subscription, which causes us to have other test failures due to hitting our limits).
I've left some comments inline - if you can fix them up (and the tests pass) we should be able to merge this particular instance; that said, I don't expect us to merge any additional fixes/changes to Application Gateway's until the API issue outlined here is fixed: Azure/azure-rest-api-specs#2187
Thanks!
| Type: schema.TypeList, | ||
| Optional: true, | ||
| Elem: &schema.Schema{ | ||
| Type: schema.TypeString, |
There was a problem hiding this comment.
given this is a status code, can we make this a TypeInt?
There was a problem hiding this comment.
People can specify status codes as ranges also like 200-300.
| if match := props.Match; match != nil { | ||
|
|
||
| matchConfig := map[string]interface{}{ | ||
| "body": *match.Body, |
There was a problem hiding this comment.
there's a potential crash here if body is nil - can we make this:
matchConfig := map[string]interface{}{}
if body := match.Body; body != nil {
matchConfig["body"] = *body
}
There was a problem hiding this comment.
Thanks. Fixed it.
| } | ||
|
|
||
| statusCodes := make([]interface{}, 0) | ||
| for _, status := range *match.StatusCodes { |
There was a problem hiding this comment.
there's a potential crash here if the API returns a bad response, can we make this:
if match.StatusCodes != nil {
for _, status := ...
}
| @@ -457,6 +457,29 @@ func resourceArmApplicationGateway() *schema.Resource { | |||
| Type: schema.TypeInt, | |||
| Required: true, | |||
| }, | |||
|
|
|||
| "match": { | |||
There was a problem hiding this comment.
can we also add documentation for this block?
| 203, | ||
| 202, | ||
| ] | ||
| } |
There was a problem hiding this comment.
since this is an optional block, can we add a test specifically for this?
There was a problem hiding this comment.
I have added a separate test that verifies the presence of Match block in the probe is being set.
|
@tombuildsstuff Thanks for your review. This is the last fix I will make in Application gateway and this was blocking the customer I was working with. Currently they are working around this using powershell and wanted to fix this ASAP. I have addressed all the review comments. |
|
The Application Gateway tests are intermittent - but this seems fine:
|
|
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 👉 hashibot-feedback@hashicorp.com. Thanks! |
ghost
locked and limited conversation to collaborators
This is blocking a current customer. I have added the change to existing test case probe. Ran the tests private but failed on deleting subnet which is a known issue.