Deleting unecessary app settings that replace existing infra app sett… - #2494
Conversation
…ings in the appService.bicep + set those app settings as variable in build & deploy pipeline
|
/deploy |
There was a problem hiding this comment.
Pull request overview
This PR removes App Service siteConfig.appSettings management from the Bicep module to prevent infrastructure deployments from overwriting (replacing) runtime/app secrets that are merged in via CI/CD, and shifts the essential baseline settings into the deployment workflows.
Changes:
- Remove
APPLICATIONINSIGHTS_CONNECTION_STRINGparameter/appSettings frominfra/modules/appService.bicep. - Stop passing the App Insights connection string into the App Service module from
infra/main.bicep. - Add baseline App Service settings (storage, port, AI connection string, docker registry URL) to the GitHub Actions deployment workflows.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| infra/modules/appService.bicep | Removes siteConfig.appSettings to avoid ARM “replace” behavior wiping pipeline-managed env vars. |
| infra/main.bicep | Removes now-unused module parameter wiring for app settings. |
| .github/workflows/pr-deploy.yml | Sets baseline app settings via az webapp config appsettings set for PR slots. |
| .github/workflows/build-and-deploy.yml | Sets baseline app settings via az webapp config appsettings set for staging/prod deployments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| --settings | ||
| "WEBSITES_ENABLE_APP_SERVICE_STORAGE=false" | ||
| "DOCKER_REGISTRY_SERVER_URL=https://${{ needs.infrastructure.outputs.acrName }}.azurecr.io" | ||
| "WEBSITES_PORT=3000" | ||
| "APPLICATIONINSIGHTS_CONNECTION_STRING=${{ needs.infrastructure.outputs.appInsightsConnectionString }}" | ||
| "TINA_WEBHOOK_SECRET=${{ secrets.TINA_WEBHOOK_SECRET }}" |
There was a problem hiding this comment.
The base App Service settings (WEBSITES_ENABLE_APP_SERVICE_STORAGE, WEBSITES_PORT, APPLICATIONINSIGHTS_CONNECTION_STRING, etc.) are now duplicated across multiple workflows. Consider centralizing these shared settings into a reusable composite action or a repo script so future changes don’t drift between PR previews and main deployments.
PR Preview Deployed
This preview will be automatically deleted when the PR is closed. |
|
@PothieuG Just check the comment I left. Otherwise this looks fine. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
/deploy |
PothieuG
changed the title
Description
PBI - #2456
What happened:
appSettingscollection with these above 4 values, wipping all the other environements variables.After investigation, those 4 settings are not mandatory there:
acrUseManagedIdentityCreds: trueis set.80. Can be set at any point before the container starts.true. Setting it tofalseis a best practice for containers but the app still works either way.They are all runtime configuration and not a prerequisite for infrastructure.
So I deleted them from the
appService.bicepfile and managed them directly in the pipeline viaaz webapp config appsettings set(which merges instead of replacing like in the bicep file).