-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Adds support to specify linux_fx_version for Function Apps #2767
Adds support to specify linux_fx_version for Function Apps #2767
Conversation
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.
Hi @joakimhew,
Thanks for the enhancement! This looks goo to me aside from a missing test, could we add it to one much like the linux_fx_version
is tested for app service?
I suggest adding it to TestAccAzureRMFunctionApp_siteConfig
and one of TestAccAzureRMFunctionApp_siteConfigMulti
.
Once we have a test this should be good to merge 🙂 Thanks!
@katbyte Thank you for the review! Should've thought of that of course haha. I'll correct it later and update my PR 👍 |
@katbyte I'll add EDIT: After further investigation this may be computed somehow. I've tried setting EDIT 2: App Service Plan resource:
Function App resource: |
390f251
to
8c85f8e
Compare
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.
Hi @joakimhew,
Thanks for the tests. I've left a few comments inline but it's looking close
@katbyte Once we've resolved the last thing, I'd like to fix the author of my commits as they are currently not set to my GitHub account. This would rewrite history on my |
@joakimhew, that is totally fine 🙂 just @me when your ready for a final review and merge! |
647010c
to
f1b34ff
Compare
Hi @katbyte! I've just set |
@@ -157,6 +157,8 @@ The following attributes are exported: | |||
|
|||
* `site_credential` - A `site_credential` block as defined below, which contains the site-level credentials used to publish to this App Service. | |||
|
|||
* `kind` - The Function App kind - such as `functionapp,linux,container` |
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.
Could we separate these values:
* `kind` - The Function App kind - such as `functionapp,linux,container` | |
* `kind` - The Function App kind - such as `functionapp`, `linux`, `container` |
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.
Sure I could do that, this is how Azure will write it back to the user though. Should I still separate it? (This is an example of an function app with multiple kinds so its actually a valid kind 😀)
Edit:
I think the possible computed kinds for a Function App right now is;
functionapp
functionapp,linux,container
There might be more combinations but I’m not entirely sure. Microsoft’s documentation on this is very lacking unfortunately
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.
Hi @joakimhew,
Thanks for the updates, this LGTM now 🚀
tests:
|
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! |
This PR fixes 1937 and allows users to specify the
linux_fx_version
for azure function apps (resource_arm_function_app.go)For consistency, I think
linux_fx_version
is a better option than the alternative mentioned in the issue, aslinux_fx_version
is already used in App Services.