-
Notifications
You must be signed in to change notification settings - Fork 352
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
[Feature request from Discussions] RFC: Microsoft.Web/sites should retain existing app configuration over deployment #949
Comments
Here is my first untested idea for controlling which app config values are being retained: diff --git a/FunctionApp/template/Microsoft.Web/sites/config-appsettings/deploy.bicep b/FunctionApp/template/Microsoft.Web/sites/config-appsettings/deploy.bicep
index ceb99ff..205aa0c 100644
--- a/FunctionApp/template/Microsoft.Web/sites/config-appsettings/deploy.bicep
+++ b/FunctionApp/template/Microsoft.Web/sites/config-appsettings/deploy.bicep
@@ -32,6 +32,12 @@ param enableDefaultTelemetry bool = true
@description('Optional. Existing function app configuration')
param existingConfig object = {}
+param retainAppSetting string
+
+param retainAppSettingsArray array
+
+param appSettingsKeyValuePairsOverridesExistingConfig bool
+
// =========== //
// Variables //
// =========== //
@@ -46,13 +52,13 @@ var appInsightsValues = !empty(appInsightId) ? {
APPLICATIONINSIGHTS_CONNECTION_STRING: appInsight.properties.ConnectionString
} : {}
-var filteredExistingConfig_ = [for item in items(existingConfig): item.key != 'APPINSIGHTS_INSTRUMENTATIONKEY' && item.key != 'APPLICATIONINSIGHTS_CONNECTION_STRING' && item.key != 'AzureWebJobsStorage' && item.key != 'AzureWebJobsDashboard' ? {
+var filteredExistingConfig_ = [for item in items(existingConfig): (retainAppSetting == 'retainAppSetting' && contains(retainAppSettingsArray, item.key) ) && item.key != 'APPINSIGHTS_INSTRUMENTATIONKEY' && item.key != 'APPLICATIONINSIGHTS_CONNECTION_STRING' && item.key != 'AzureWebJobsStorage' && item.key != 'AzureWebJobsDashboard' ? {
'${item.key}': item.value
} : {}]
var filteredExistingConfig = reduce(filteredExistingConfig_, {}, (cur, next) => union(cur, next))
-// TODO: add parameter to control appSettingsKeyValuePairs or filteredExistingConfig has higher preference
-var expandedAppSettings = union(appSettingsKeyValuePairs, filteredExistingConfig, azureWebJobsValues, appInsightsValues)
+// order of the filteredExistingConfig and appSettingsKeyValuePairs is controlled by appSettingsKeyValuePairsOverridesExistingConfig
+var expandedAppSettings = appSettingsKeyValuePairsOverridesExistingConfig ? union(filteredExistingConfig, appSettingsKeyValuePairs, azureWebJobsValues, appInsightsValues) : union(appSettingsKeyValuePairs, filteredExistingConfig, azureWebJobsValues, appInsightsValues)
// =========== //
// Existing resources //
diff --git a/FunctionApp/template/Microsoft.Web/sites/deploy.bicep b/FunctionApp/template/Microsoft.Web/sites/deploy.bicep
index 5c9a617..b182874 100644
--- a/FunctionApp/template/Microsoft.Web/sites/deploy.bicep
+++ b/FunctionApp/template/Microsoft.Web/sites/deploy.bicep
@@ -64,6 +64,16 @@ param appSettingsKeyValuePairs object = {}
@description('Optional. The auth settings V2 configuration.')
param authSettingV2Configuration object = {}
+@description('Optional. Retain existing app config')
+@allowed(['None', 'Selected', 'All'])
+param retainAppSetting string = contains(kind, 'functionapp') ? 'Selected' : 'None'
+
+@description('Optional. List of retained app config keys. Used when retainAppSetting = \'Selected\'')
+param retainAppSettingsArray array = contains(kind, 'functionapp') ? ['WEBSITE_RUN_FROM_PACKAGE'] : []
+
+@description('a')
+param appSettingsKeyValuePairsOverridesExistingConfig bool = false
+
// Lock
@allowed([
''
@@ -221,7 +231,7 @@ resource app 'Microsoft.Web/sites@2021-03-01' = {
}
}
-module existingConfig 'config-appsettings/existing-config.bicep' = {
+module existingConfig 'config-appsettings/existing-config.bicep' = if (retainAppSetting != 'None') {
name: '${uniqueString(deployment().name, location)}-Existing-Site-Config'
params: {
parent: app.name
@@ -238,7 +248,10 @@ module app_appsettings 'config-appsettings/deploy.bicep' = if (!empty(appSetting
setAzureWebJobsDashboard: setAzureWebJobsDashboard
appSettingsKeyValuePairs: appSettingsKeyValuePairs
enableDefaultTelemetry: enableReferencedModulesTelemetry
- existingConfig: existingConfig.outputs.existingConfig.properties
+ existingConfig: retainAppSetting != 'None' ? existingConfig.outputs.existingConfig.properties : { }
+ retainAppSetting: retainAppSetting
+ retainAppSettingsArray: retainAppSettingsArray
+ appSettingsKeyValuePairsOverridesExistingConfig: appSettingsKeyValuePairsOverridesExistingConfig
}
} This should add some basic control for app config retain:
Microsoft.Web/sites/deploy.bicep three more parameters:
Support for slots is still missing. Is there something else important missing or does anyone see problems with the current PoC. Now default setting will retain WEBSITE_RUN_FROM_PACKAGE app config for functions apps. I'm not sure if that should be turned on by default or not. |
my current solution is: if the resource exist i'm running a module which gets the existing function with appconfig if it is false ill output an empty object which merges with the app config |
Hey @jikuja I wonder how complicated this may become given the vast amount of possible app settings (which are already split across multiple child items). Reading the description and reasoning for the change I wonder if an alternative pattern wouldn't be to split the function app deployment (as part of a solution/workload) into its own pipeline to allow an independent update of the function app settings (& code). As you'd still need reference to resources you could then leverage 'exiting' resources instead. This is not to say there is no merit in adding this 'only' update mode, but I have to wonder how 'advanced' is too advanced. This would become a lot easier once #4023 is implemented. Just adding it here for reference. |
@tsc-buddy Has there been any discussions if/when problems mentinoed here could be fixed by RP instead of extra templating logic layers? Highes priority should for |
Just enquiring if there's an update on this? Like others, my Linux consumption function app gets deleted when bicep is redeployed. I'm in the process of adding functions and as part of that I need to create additional app settings. Once I deploy all the functions are deleted. I've tried setting and removing We're about to go live with a bunch of webhook handlers which run 24/7. I'm now afraid to put this into production for fear of accidental deletion. FWIW we deploy our node apps with GHA |
It might be a good idea to
The fix I would like to see is in the first message under the title |
you can also look at: |
Hola together, That being said, I agree that the RP doesn't exactly make it easy to implement. However, implementing that layer should be technically possible - and given that each module in AVM has voluntary owners that are assigned the issues and handle them, we should see some traction there too. Whether that's an implementation or inquiry to the PG(s) is up to the corresponding owner. Last but not least, as the Web/sites module is already migrated, I'll go ahead and move this issue now. Will cc you to make sure you can find it after. |
@tsc-buddy, please note that I just transfered this issue from the original CARML repository. Would be great if you could take a look once you have a moment :) |
Hey @AlexanderSehr - sure thing. I shall schedule some time aside to take a look this in the coming week. |
Much appreciated, thanks. If you need any support, let me/us know. |
This will be scheduled for review in the Month or March 2024. |
Thanks @tsc-buddy, sorry the bot is very very persisent. @matebarabas would it be a possiblilty to reduce it's schedule? Some items have so many of these bot comments... |
We just hit this issue in a customer project, so I'm also very interested in a fix. |
Hi @krbar, this article might be able to help you: https://blog.dotnetstudio.nl/posts/2021/04/merge-appsettings-with-bicep/ I think I've ran into the same issues that you've experienced and following the previous article helped me keep the old configuration settings. |
Looks promising, thank you @EverybodyKurts! @AlexanderSehr, @tsc-buddy this looks like a good inspiration on how we could implement it in AVM, what do you think? |
Looks really similar with mine. Using
First thing:
Then to plan how settings are merged?
|
But does it work if the app service is deployed for the very first time? (So no list possible) |
Fair point 😄 . @jikuja, any experience with that? It it works in any way like the |
Good points here. I tested Schutten's code and works even on first deployment
Existing keyword documentation clearly states that deployment will fail when referenced resource is not found: If non-accessor typed list*() invocations are called when resource is being deployed then function app and its @alex-frankel Do you want to comment if we are trying to build something that is working by luck or is it working by design? Main points starts from the comment #949 (comment) Understanding exact details of IL is kind of black magic. |
Regarding |
…#3311) ## Description This commit changes the app settings deployment in order to retain existing app settings that are not defined in the Bicep file. This change allows for updating **only** the app settings that are defined in the Bicep file, while leaving the rest unchanged. As this is a change in behavior, version number has been increased. Fixes #949 ## Pipeline Reference <!-- Insert your Pipeline Status Badge below --> | Pipeline | | -------- | | [![avm.res.web.site](https://github.com/peterbud/bicep-registry-modules/actions/workflows/avm.res.web.site.yml/badge.svg)](https://github.com/peterbud/bicep-registry-modules/actions/workflows/avm.res.web.site.yml) | ## Type of Change <!-- Use the checkboxes [x] on the options that are relevant. --> - [ ] Update to CI Environment or utilities (Non-module affecting changes) - [ ] Azure Verified Module updates: - [ ] Bugfix containing backwards-compatible bug fixes, and I have NOT bumped the MAJOR or MINOR version in `version.json`: - [ ] Someone has opened a bug report issue, and I have included "Closes #{bug_report_issue_number}" in the PR description. - [ ] The bug was found by the module author, and no one has opened an issue to report it yet. - [x] Feature update backwards compatible feature updates, and I have bumped the MINOR version in `version.json`. - [ ] Breaking changes and I have bumped the MAJOR version in `version.json`. - [ ] Update to documentation ## Checklist - [x] I'm sure there are no other open Pull Requests for the same update/change - [x] I have run `Set-AVMModule` locally to generate the supporting module files. - [x] My corresponding pipelines / checks run clean and green without any errors or warnings
Discussed in Azure/ResourceModules#2572
Originally posted by jikuja January 14, 2023
I would like to start a discussion if
Microsoft.Web/sites
should retain existing app config and I'll provide a quick proof-of-concept for implementation ideas.Why to retain app config
Why not to retain app config
The proper solution
Resource provider should have two set of configuration:
Solution with templating
Proof-of-concept
This is really naive solution to persist app config over deployment using Bicep only:
Proof-of-concept explanation
deploy.bicep
fetch existing configuration withconfig-appsettings/existing-config.bicep
moduleconfig-appsettings/deploy.bicep
config-appsettings/deploy.bicep
:config-appsettings/deploy.bicep
Open questions
WEBSITE_RUN_FROM_PACKAGE
cc: @jikuja, @mikkark
The text was updated successfully, but these errors were encountered: