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_application_gateway: Support for Hostname #2990

Merged
merged 4 commits into from
Mar 21, 2019
Merged

azurerm_application_gateway: Support for Hostname #2990

merged 4 commits into from
Mar 21, 2019

Conversation

mcharriere
Copy link
Contributor

Added support for hostname in backend http settings. #1576

metacpp
metacpp previously approved these changes Mar 5, 2019
Copy link
Contributor

@metacpp metacpp left a comment

Choose a reason for hiding this comment

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

@mcharriere Thanks for the contribution, mostly it LGTM, please address comments in the PR.

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.

Hi @mcharriere,

Thank you for the contribution! I've taken a look and left some comments inline. My main concern is always setting the value in the API, typically for optional values we won't set the field in the request if the property isn't set in tf.

azurerm/resource_arm_application_gateway.go Show resolved Hide resolved
azurerm/resource_arm_application_gateway.go Outdated Show resolved Hide resolved
azurerm/resource_arm_application_gateway.go Outdated Show resolved Hide resolved
azurerm/resource_arm_application_gateway.go Outdated Show resolved Hide resolved
@@ -260,6 +260,32 @@ func TestAccAzureRMApplicationGateway_probesPickHostNameFromBackendHTTPSettings(
})
}

func TestAccAzureRMApplicationGateway_backendHttpSettingsHostName(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we get another test with 3 steps:

  • set pick true
  • set hostname
  • unset hostname

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a doubt regarding the test. If I set pick true and then I try to set hostname it will throw an error. Until here it's fine, i can check for that error. But I dont understand the following step. I think that unsetting hostname won't do anything because of the previous error.

Should I add a test just to check for that error (both pick and host_name set)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@katbyte I've added the test TestAccAzureRMApplicationGateway_backendHttpSettingsHostNameAndPick checking for the error and it's passing.
Please let me know if you think there is something else to check for.

@katbyte
Copy link
Collaborator

katbyte commented Mar 5, 2019

As suspected i think always setting the property causes issues:

------- Stdout: -------
=== RUN   TestAccAzureRMApplicationGateway_probesPickHostNameFromBackendHTTPSettings
=== PAUSE TestAccAzureRMApplicationGateway_probesPickHostNameFromBackendHTTPSettings
=== CONT  TestAccAzureRMApplicationGateway_probesPickHostNameFromBackendHTTPSettings
--- FAIL: TestAccAzureRMApplicationGateway_probesPickHostNameFromBackendHTTPSettings (107.04s)
    testing.go:538: Step 0 error: Error applying: 1 error occurred:
        	* azurerm_application_gateway.test: 1 error occurred:
        	* azurerm_application_gateway.test: Error Creating/Updating Application Gateway "acctestag-190305192949044967" (Resource Group "acctestRG-190305192949044967"): network.ApplicationGatewaysClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="ApplicationGatewayBackendHttpSettingsHostNameFieldConflict" Message="HostName field may only be specified if pickHostNameFromBackendAddress is set to false in context '/subscriptions/c0a607b2-6372-4ef3-abdb-dbe52a7b56ba/resourceGroups/acctestRG-190305192949044967/providers/Microsoft.Network/applicationGateways/acctestag-190305192949044967/backendHttpSettingsCollection/acctest-vnet-190305192949044967-be-htst'." Details=[]

@mcharriere
Copy link
Contributor Author

Oh @katbyte thanks for pointing out that. I'll fix it soon.

@ghost ghost removed the waiting-response label Mar 5, 2019
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 for the updates @mcharriere!

this LGTM now 👍

@katbyte katbyte merged commit 858c38e into hashicorp:master Mar 21, 2019
@katbyte katbyte added this to the v1.24.0 milestone Mar 21, 2019
katbyte added a commit that referenced this pull request Mar 21, 2019
@ghost
Copy link

ghost commented Apr 3, 2019

This has been released in version 1.24.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
	version = "~> 1.24.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Apr 21, 2019

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 Apr 21, 2019
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.

4 participants