feat: Retain existing settings during deployment - avm/res/web/site
#3311
Conversation
This commit adds the retainExistingSettings parameter to the app settings deployment in order to retain existing app settings that are not defined in the Bicep file. This parameter allows for updating only the app settings that are defined in the Bicep file, while leaving the rest unchanged. Fixes Azure#949
|
Important The "Needs: Triage 🔍" label must be removed once the triage process is complete! Tip For additional guidance on how to triage this issue/PR, see the BRM Issue Triage documentation. |
|
Important If this is a module-related PR, being submitted by the sole owner of the module, the AVM core team must review and approve it (as module owners can't approve their own PRs). To indicate this PR needs the core team''s attention, apply the "Needs: Core Team 🧞" label! The core team will only review and approve PRs that have this label applied! |
avm/res/web/site
avm/res/web/siteavm/res/web/site
| @@ -294,6 +294,7 @@ module app_appsettings 'config--appsettings/main.bicep' = if (!empty(appSettings | |||
| storageAccountUseIdentityAuthentication: storageAccountUseIdentityAuthentication | |||
| appInsightResourceId: appInsightResourceId | |||
| appSettingsKeyValuePairs: appSettingsKeyValuePairs | |||
| currentAppSettings: !empty(app.id) ? list('${app.id}/config/appsettings', '2023-12-01').properties : {} | |||
There was a problem hiding this comment.
What kind of deployment would have an empty app.id?
| @@ -294,6 +294,7 @@ module app_appsettings 'config--appsettings/main.bicep' = if (!empty(appSettings | |||
| storageAccountUseIdentityAuthentication: storageAccountUseIdentityAuthentication | |||
| appInsightResourceId: appInsightResourceId | |||
| appSettingsKeyValuePairs: appSettingsKeyValuePairs | |||
| currentAppSettings: !empty(app.id) ? list('${app.id}/config/appsettings', '2023-12-01').properties : {} | |||
There was a problem hiding this comment.
Should this be configurable? I fear that someone will open a ticket at some point to have full idempotency.
There was a problem hiding this comment.
@jikuja Certainly yes. IaC is about desired state and full update not incremental.
But I'm more concerned with a potential race condition.
There was a problem hiding this comment.
@jikuja Can you elaborate on what do you think when you say race condition ?
There was a problem hiding this comment.
@jikuja Can you elaborate on what do you think when you say race condition ?
@pankajagrawal16 It seems the deployment is doing a fetch, then merge the properties. It is not an atomic operation. Two concurrent deployments (with different name) will fetch the same old values and only pushes its new properties. The last one to push will wins.
There was a problem hiding this comment.
@scrocquesel-ml150 Are you talking about multiple concurrent deployments of avm/res/web/site/main.bicep or concurrent deployment of avm/res/web/site/main.bicep main module and avm/res/web/site/config--appsettings/main.bicep?
There was a problem hiding this comment.
@scrocquesel-ml150 Are you talking about multiple concurrent deployments of
avm/res/web/site/main.bicepor concurrent deployment ofavm/res/web/site/main.bicepmain module andavm/res/web/site/config--appsettings/main.bicep?
two avm/res/web/site/main.bicep will fetch the same existing values, then, after resource app is completed, each one will create a new avm/res/web/site/config--appsettings/main.bicep deployment with the same existing values and maybe different new values as appSettingsKeyValuePairs. I think if you want to be sure no race condition can occur, you can make an atomic deployment with the exact same deployment name because Azure will not permit to create a deployment of the same name if one exists and is not completed and will fail the deployment. So if you fetch the existing value and update the properties inside such a deployment, you have kind of a poor-man semaphore/lock.
I admit this is a race condition that is difficult to produce and everyone is using more or less this pattern without even knowing it exists. I guess it is a matter of complexity vs reliability.
There was a problem hiding this comment.
I don't see the issue you are describing here.
In case one is running the same deployments concurrently (it's already an anti pattern AFAIK) they will get the same appSettingsKeyValuePairs (i.e.: keeping values NOT defined in the bicep file, and merging them with the defined values from the bicep file) - so I don't see an issue or race condition: both deployment will result the same result even running concurrently.
In case somebody is running different deployments concurrently, then the last will win, aka the latest deployment will take precedence - which is normal. Also note, that was the behavior before this change already.
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
Type of Change
version.json:version.json.version.json.Checklist
Set-AVMModulelocally to generate the supporting module files.