-
Notifications
You must be signed in to change notification settings - Fork 393
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
feat: Retain existing settings during deployment - avm/res/web/site
#3311
Changes from all commits
c0fc692
217e9a1
a53cdcd
8a65891
09dd0dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -245,7 +245,7 @@ resource avmTelemetry 'Microsoft.Resources/deployments@2024-03-01' = if (enableT | |
} | ||
} | ||
|
||
resource app 'Microsoft.Web/sites@2022-09-01' = { | ||
resource app 'Microsoft.Web/sites@2023-12-01' = { | ||
name: name | ||
location: location | ||
kind: kind | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jikuja Certainly yes. IaC is about desired state and full update not incremental. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jikuja Can you elaborate on what do you think when you say race condition ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @scrocquesel-ml150 Are you talking about multiple concurrent deployments of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
two There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. |
||
} | ||
} | ||
|
||
|
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.
What kind of deployment would have an empty app.id?