azurerm_linux_web_app, azurerm_linux_web_app_slot - fix docker setting not being saved to application setting issue
#24086
Conversation
There was a problem hiding this comment.
Hi @xiaxyi - Thanks for this PR, I think this might need a broader fix to avoid other potential similar problems, suggestion below if you can take a look?
Thanks!
| if siteConfigAppSetting := siteConfig.AppSettings; siteConfigAppSetting != nil && len(*siteConfigAppSetting) > 0 { | ||
| if webApp.AppSettings == nil { | ||
| webApp.AppSettings = make(map[string]string) | ||
| } | ||
| for _, pair := range *siteConfigAppSetting { | ||
| if pair.Name == nil || pair.Value == nil { | ||
| continue | ||
| } | ||
| if *pair.Name == "DOCKER_REGISTRY_SERVER_URL" || | ||
| *pair.Name == "DOCKER_REGISTRY_SERVER_USERNAME" || | ||
| *pair.Name == "DOCKER_REGISTRY_SERVER_PASSWORD" { | ||
| webApp.AppSettings[*pair.Name] = *pair.Value | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this only address part of a larger problem here? Other evaluated settings may also be dropped from the map? I suspect what we should do here is extend the func ExpandAppSettingsForUpdate to take both maps and combine them to ensure everything is caught?
There was a problem hiding this comment.
Any KVP that are updated via the siteConfig api needs to be updated using the APPSETTING dedicated POST API(https://learn.microsoft.com/en-us/rest/api/appservice/web-apps/list-application-settings?view=rest-appservice-2022-03-01). I only updated the docker one to follow one issue in one PR.
You are correct about extending the ExpandAppSettingsForUpdate to merge the KVPs
|
updated as the comment |
|
As discussed offline with @xiaxyi - closing in favour of 24221 |
|
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. |
The same issue as the previous PR:#22484
As already mentioned in that pr, there are two api to set the application setting:
POST "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Web/sites/{name}/config/appsetting
In current provider, we only update the app setting via #1 which takes no effect to the app setting block. We need to update the docker setting using #2.
fix #24046
fix #23632
fix #23525
fix #22379