Preserve Dapr configuration on container app deployment#7062
Merged
Conversation
1 task
Co-authored-by: spboyer <7681382+spboyer@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix dapr configuration removal during azd up deployment
Preserve Dapr configuration on container app deployment
Mar 11, 2026
spboyer
requested review from
JeffreyCA,
hemarina,
rajeshkamal5050,
tg-msft,
vhvb1989,
wbreza and
weikanglim
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes azd deploy behavior for Azure Container Apps so externally-configured Dapr settings (e.g., via Terraform) aren’t silently removed when the generated deployment YAML omits properties.configuration.dapr.
Changes:
- Add a Dapr config path constant and extend
persistSettingsto copy live Dapr config into the outgoing payload when YAML omits it. - Add tests validating (1) Dapr preservation when YAML omits Dapr, and (2) YAML-provided Dapr overrides existing config.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cli/azd/pkg/containerapps/container_app.go |
Preserves existing Dapr configuration by copying it from the live resource when YAML doesn’t specify Dapr. |
cli/azd/pkg/containerapps/container_app_test.go |
Adds unit tests for Dapr preservation and override behavior during DeployYaml. |
Comments suppressed due to low confidence (2)
cli/azd/pkg/containerapps/container_app.go:187
- The PR description calls out that a first deploy (GET 404) should be handled gracefully. With the new default Dapr-preservation path,
persistSettingswill now perform a GET even when no alpha flags are enabled, and it will log an error on 404. It would be better to detectazcore.ResponseErrorwithStatusCode == 404and skip the log (and just return the YAML unchanged).
aca, err := cas.getContainerApp(ctx, subscriptionId, resourceGroupName, appName, options)
if err != nil {
log.Printf("failed getting current aca settings: %v. No settings will be persisted.", err)
// if the container app doesn't exist, there's nothing for us to update in the desired state,
// so we can just return the existing state as is.
return obj, nil
cli/azd/pkg/containerapps/container_app_test.go:922
- Given the risk of unintentionally removing existing Dapr settings if the GET fails, it would be good to add a test that simulates a non-404 GET failure (e.g. 500) when YAML omits
dapr, and verifies the deploy behavior you want (ideally returning an error rather than proceeding with a destructive PUT).
func Test_ContainerApp_DeployYaml_PreservesDaprConfig(t *testing.T) {
mockContext := mocks.NewMockContext(context.Background())
subscriptionId := "SUBSCRIPTION_ID"
location := "eastus2"
resourceGroup := "RESOURCE_GROUP"
appName := "APP_NAME"
// YAML does NOT include Dapr configuration
containerAppYaml := `
location: eastus2
name: APP_NAME
properties:
configuration:
activeRevisionsMode: Single
template:
containers:
- image: IMAGE_NAME
`
// Existing container app has Dapr enabled
existingApp := &armappcontainers.ContainerApp{
Location: to.Ptr(location),
Name: to.Ptr(appName),
Properties: &armappcontainers.ContainerAppProperties{
Configuration: &armappcontainers.Configuration{
ActiveRevisionsMode: to.Ptr(armappcontainers.ActiveRevisionsModeSingle),
Dapr: &armappcontainers.Dapr{
AppID: to.Ptr("my-app"),
AppPort: to.Ptr[int32](8080),
Enabled: to.Ptr(true),
},
},
Template: &armappcontainers.Template{
Containers: []*armappcontainers.Container{
{
Image: to.Ptr("IMAGE_NAME"),
},
},
},
},
}
_ = mockazsdk.MockContainerAppGet(mockContext, subscriptionId, resourceGroup, appName, existingApp)
containerAppUpdateRequest := mockazsdk.MockContainerAppCreateOrUpdate(
mockContext, subscriptionId, resourceGroup, appName, existingApp,
)
cas := NewContainerAppService(
mockContext.SubscriptionCredentialProvider,
clock.NewMock(),
mockContext.ArmClientOptions,
mockContext.AlphaFeaturesManager,
)
err := cas.DeployYaml(*mockContext.Context, subscriptionId, resourceGroup, appName, []byte(containerAppYaml), nil)
require.NoError(t, err)
var actual *armappcontainers.ContainerApp
err = mocks.ReadHttpBody(containerAppUpdateRequest.Body, &actual)
require.NoError(t, err)
// Dapr configuration should be preserved from the existing container app
require.NotNil(t, actual.Properties.Configuration.Dapr)
require.Equal(t, "my-app", *actual.Properties.Configuration.Dapr.AppID)
require.Equal(t, int32(8080), *actual.Properties.Configuration.Dapr.AppPort)
require.Equal(t, true, *actual.Properties.Configuration.Dapr.Enabled)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rajeshkamal5050
approved these changes
Mar 11, 2026
Collaborator
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
Member
Review feedbackHey @spboyer 👋 — thanks for tackling this! The fix is correct and the tests are solid. Preserving Dapr config set via Terraform (or other external tools) is absolutely the right thing to do. That said, I want to flag a design concern about the approach before we merge. The "illusion" problemToday, This PR makes Dapr preservation always-on with no feature flag, which breaks that pattern. While it solves the immediate problem, it could create an illusion for customers that How would a user know which configuration is respected by Questions
Not blocking — just want to make sure we're intentional about the precedent this sets. 🙏 |
weikanglim
approved these changes
Mar 11, 2026
wbreza
approved these changes
Mar 11, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
📋 Code Review — PR #7062
Clean, focused correctness fix. Well-implemented and follows existing persist-settings patterns.
✅ What Looks Good
- Follows existing
persistDomainsandpersistIngressSessionAffinitypatterns - Correct decision not to feature-flag this — it's a correctness fix
- Logic is sound: YAML has Dapr → use it; YAML missing Dapr → preserve existing
- Two complementary tests covering preserve and override scenarios
Minor gap: No test for first-deploy (404 from GET) case, but that path is shared pre-existing code.
Overall Assessment: Approve
Member
|
Thanks for the thorough review @vhvb1989 — these are the right questions to ask. Let me address each: 1. Is there an expectation to expand this pattern? Yes — the existing 2. Should this be behind an alpha feature flag? I'd argue no, and here's why the Dapr case is different from domains/sessions:
The key distinction: we're not adding functionality (opt-in), we're preventing data loss (correctness). The code only preserves Dapr when the deployment YAML doesn't specify it — if the YAML explicitly sets Dapr config, that takes precedence. 3. Does this apply to other hosts? Good question. Today this is Container Apps-specific because ACA's 4. Could a general "preserve all unspecified config" approach work? This is the ideal end state — a deep-merge strategy where the YAML is overlaid on existing config rather than replacing it wholesale. That's a larger effort and changes the deployment contract significantly. This PR is a targeted fix for the most impactful case (Dapr breaks the app), and a general merge strategy would be a great follow-up. Summary: I see this as a correctness fix (preventing silent destructive behavior) rather than a new feature (which should be opt-in). Happy to add a comment in the code explaining why Dapr preservation is always-on vs. the alpha-gated patterns, to make the reasoning visible for future maintainers. |
- Handle 404 (first deploy) explicitly in persistSettings instead of swallowing all errors — proceed without persisting when app does not exist yet - Fail on non-404 errors when Dapr preservation is needed to prevent silent config wipe (correctness-critical path) - Add test for first-deploy scenario (GET 404) verifying no Dapr config is injected - Fix pre-existing cspell issues (projectpkg, agentserver) - Apply go fix modernizations (to.Ptr -> new) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
spboyer
force-pushed
the
copilot/fix-dapr-configuration-issue
branch
from
a1c1b8e to
113829d
Compare
rajeshkamal5050
approved these changes
Mar 12, 2026
vhvb1989
approved these changes
Mar 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When
azd deployruns against a container app whose Dapr sidecar was configured externally (e.g., via Terraform), theCreateOrUpdatePUT operation replaces the full configuration, silently removing Dapr settings absent from the Aspire-generated YAML.Changes
pkg/containerapps/container_app.gopathConfigurationDapr = "properties.configuration.dapr"path constantpersistSettingsto detect whether the deployment YAML containsproperties.configuration.dapr; if absent, fetches the live container app and copies its Dapr config into the outgoing payload before the PUT — no feature flag required since this is a correctness fixpersistDomains,persistIngressSessionAffinity) are unaffectedpkg/containerapps/container_app_test.goTest_ContainerApp_DeployYaml_PreservesDaprConfig: YAML without Dapr → existing Dapr config is preservedTest_ContainerApp_DeployYaml_YamlDaprConfigNotOverridden: YAML with explicit Dapr config → YAML value wins over existing app configBehavior
daprsection, app has Dapr configureddaprsection✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.