azurerm_linux_function_app azurerm_linux_function_app_slot - fixed wrong removal of WEBSITE_RUN_FROM_PACKAGE

[gioleppe]

As per the title, I fixed the case in which WEBSITE_RUN_FROM_PACKAGE gets removed if not set explicitly in the appsettings.

This was breaking deployments as the provider silently removed the setting even if it was explicitly ignored with ignore_settings, as documented on #17081 which this PR aims to fix

There was already a PR but it was closed due to inactivity

With this PR, it's now sufficient to ignore the WEBSITE_RUN_FROM_PACKAGE setting, as ADO(/cli/whatever are using to do the runfrompackage deployment) is setting it up for the user.
Removal of locs fixing this issue was suggested by @jackofallops in the discussion under the original PR

fix #17081

[Sefriol]

I would double check that this still implements the original intent of @jackofallops 's commit: 0c56e13

I guess the problem persists with app slot as well.

[gioleppe]

Hi @Sefriol. Thank you for pointing this out.

I think that what @jackofallops was trying to achieve with that commit was to filter out the setting in the model if it was been set by an external source like function app tools. This works as long as no other changes to the function app (or function app slot) are made.

As soon as you make other changes to the resource configuration, the API will be called on a model with that setting filtered out, thus breaking deployments.
This, as already discussed in #17081, subtly makes you think that you are not touching the WEBSITE_RUN_FROM_PACKAGE setting when you run terraform plan. Also, check this comment for reference.

My take on this matter is that there should not be a special treatment for this setting: if you know how to use the lifecycle meta argument you would simply ignore WEBSITE_RUN_FROM_PACKAGE from the appsettings map, and that's it.

Also, note that windows_function_app is working this way and no one ever complained about the behavior afaik.
On the other hand, I could bet this broke a lot of production deployments over the last two years (well, at least it broke mine, and the problem went undetected for a couple of days 😢)

[Sefriol]

Yeah, this broke our deployments as well. I am not disagreeing whether should be fixed. I was almost ready to make the same pull request myself. I just decided against it since I do not know the software well enough to know whether removing this could cause side effects that break current deployments.

.i.e is this a Breaking Change or not? Should there be tests which check that this works in all situations?

(While I understand that this is a bug, it could still be a breaking change that needs to be notified to the end user. Otherwise they don't know whether they need to double check their deployments)

But as said, I am not an expert in this so maybe @jackofallops or somebody qualified should review this.

[gioleppe]

First of all thanks for the feedback on this which is very much appreciated.

My take on this is that this is the contrary to a breaking change, in the sense that this PR doesn't change behavior if the setting was already present on the resource. On the other hand, if not set on the resource and not ignored in the lifecycle meta argument, now the provider would correctly show the proposed removal, which I think is the whole point of using terraform in the first place.

Even though I tried to check all relevant code paths, I'm 100% with you in thinking someone more expert than me should double check this, let's just hope it doesn't take too long since this bug has been bothering us for months now!

[gioleppe]

For what concerns tests, removing those lines makes the WEBSITE_RUN_FROM_PACKAGE setting behave just like any other setting, i.e. the behavior should be covered by existing tests
@Sefriol

[gioleppe]

@katbyte I think you've approved the tests on the latest commit, do you have any update on when this could be merged?
@jackofallops are you interested in merging this?

Thanks and have a great day!

[stephybun]

Thanks for picking this up @gioleppe! Although this is a breaking change for some users, after chatting through this internally we've decided to proceed with this PR given the nature and importance of this property for many valid deployment scenarios in App Service. We will make a point of calling this out in the Change Log. LGTM 👍

[gioleppe]

Thanks for closing this!
Super proud of my microscopic contribution to the repo :)

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.