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

Support app_command_line in App Service site_config #2350

Merged
merged 1 commit into from
Nov 19, 2018

Conversation

nickstenning
Copy link
Contributor

I took a look at fixing #2349 myself and this is my first draft! Happy to change things if I've misunderstood anything :)

--

App Service supports setting a command line to use with Linux applications (e.g. to override the default in the container image). Expose this configuration as app_command_line in site_config for azurerm_app_service.

N.B. This is also referred to as "startup file" by some sources, including the az Azure CLI and the Azure Portal.

References:

--

Closes #2349.

@ghost ghost added the size/M label Nov 17, 2018
@nickstenning
Copy link
Contributor Author

nickstenning commented Nov 17, 2018

Hmm. I've noticed that the test I added in azurerm/resource_arm_app_service_test.go doesn't actually appear to be running. If I change the assertion on line 221 the test suite still passes. 😕

This doesn't seem to be unique to my test (TestAccAzureRMAppService_appCommandLine). I can make changes that should break to, e.g., TestAccAzureRMAppService_linuxFxVersion and the suite still passes. Iiiiinteresting.

I've got to run right now but I'll look into this a bit more later on.

@nickstenning
Copy link
Contributor Author

Oh, oops! I've realised that the reason the tests weren't running was because these aren't unit tests, they're acceptance tests. Ignore my previous comment 😄

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.

hey @nickstenning

Thanks for this PR :)

I've taken a look through and left a couple of comments inline - if we can add the test/documentation for App Service Slot this otherwise LGTM 👍

Thanks!

website/docs/r/app_service.html.markdown Show resolved Hide resolved
azurerm/resource_arm_app_service_test.go Show resolved Hide resolved
App Service supports setting a command line to use with Linux
applications (e.g. to override the default in the container image).
Expose this configuration as `app_command_line` in `site_config` for
`azurerm_app_service`.

N.B. This is also referred to as "startup file" by some sources,
including the `az` Azure CLI and the Azure Portal.

References:

- https://docs.microsoft.com/en-us/rest/api/appservice/webapps/createorupdate#siteconfig
- https://docs.microsoft.com/en-us/cli/azure/webapp?view=azure-cli-latest#az-webapp-create
@nickstenning
Copy link
Contributor Author

Thanks for the speedy review. Updated with the changes you requested.

@ghost ghost removed the waiting-response label Nov 17, 2018
@nickstenning
Copy link
Contributor Author

Looks like Travis just stalled and needs a kick.

@katbyte
Copy link
Collaborator

katbyte commented Nov 18, 2018

Hi @nickstenning and @tombuildsstuff,

There currently is a PR open to a ton of the site config options to app service (#2138). It is ready to go outside of tests, however it doesn't seem like it includes app_command_line so we may want to coordinate with that PR.

@nickstenning
Copy link
Contributor Author

Thanks, @katbyte 👍. It looks like function apps also accept the --startup-file option in their config [1], which I'm assuming is backed by the same part of SiteConfig.

If this merges first, the author of that PR may want update it to include the app_command_line option. If that PR merges first, I'm happy to update this one to expose app_command_line for azurerm_function_app. Either way. 😄

  1. https://docs.microsoft.com/en-us/cli/azure/functionapp/config?view=azure-cli-latest

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.

LGTM - thanks for pushing those changes @nickstenning 👍

@tombuildsstuff
Copy link
Contributor

Tests pass, ignoring a transient failure:

screenshot 2018-11-19 at 16 37 52

@tombuildsstuff tombuildsstuff merged commit 62ba049 into hashicorp:master Nov 19, 2018
tombuildsstuff added a commit that referenced this pull request Nov 19, 2018
@ghost
Copy link

ghost commented Mar 5, 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 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 5, 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.

Support for App Service app_command_line in site_config
3 participants