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

Add health_check_path to site_config #6661

Merged
merged 4 commits into from
May 7, 2020

Conversation

sirlatrom
Copy link
Contributor

Fixes #6429.

@ghost ghost added size/XS size/M and removed size/XS labels Apr 28, 2020
@sirlatrom
Copy link
Contributor Author

@katbyte This should solve #6429. I've added an acceptance test that should pass as well. I've verified the functionality on my org's tenant, but cannot run acceptance tests due to restrictive permissions with regard to creating resource groups.

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 pr @sirlatrom,

this LGTM except for the docs need to be updated. Once thats done this should be good to merge! 👍

@katbyte katbyte added this to the v2.9.0 milestone Apr 30, 2020
@ghost ghost added the documentation label Apr 30, 2020
@sirlatrom
Copy link
Contributor Author

sirlatrom commented Apr 30, 2020

@katbyte I've added docs in 100e193. As a side note, I see some attributes are missing from the docs for the azurerm_app_service_slot page although they share the same schema for several sub-resources such as site_config, but I don't suppose adding those should be a requirement for this PR to be merged.

@ghost ghost removed the waiting-response label Apr 30, 2020
@sirlatrom sirlatrom requested a review from katbyte April 30, 2020 21:50
@sirlatrom sirlatrom force-pushed the fix-6429 branch 2 times, most recently from 02b42b4 to 330e2d9 Compare May 4, 2020 07:57
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.

one minor thing but this otherwise LGTM 👍


if input.HealthCheckPath != nil {
result["health_check_path"] = *input.HealthCheckPath
}
Copy link
Contributor

Choose a reason for hiding this comment

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

in general we need to set a value for every key inside a dictionary, which helps us catch when Azure doesn't return a value from the API (otherwise Terraform Core will show a diff between "null" and "empty") e.g.

Suggested change
}
healthCheckPath := ""
if input.HealthCheckPath != nil {
healthCheckPath = *input.HealthCheckPath
}
result["health_check_path"] = healthCheckPath

however since the rest of this class doesn't, we can ignore this for now

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 explicitly supports null as the value for this field, so I assumed that was what not setting the field would represent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW I can change the above to explicitly set result["health_check_path"] = nil when input.HealthCheckPath == nil if that is supported.

website/docs/r/app_service.html.markdown Show resolved Hide resolved
@jackofallops
Copy link
Member

image
Failures are transient on Azure side

@jackofallops jackofallops merged commit 918273d into hashicorp:master May 7, 2020
jackofallops added a commit that referenced this pull request May 7, 2020
@sirlatrom sirlatrom deleted the fix-6429 branch May 7, 2020 22:35
@ghost
Copy link

ghost commented May 8, 2020

This has been released in version 2.9.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 = "~> 2.9.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Jun 6, 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 Jun 6, 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.

Support for HealthCheckPath on SchemaAppServiceSiteConfig
4 participants