-
Notifications
You must be signed in to change notification settings - Fork 341
Fix nil panic when user declines preflight warnings #7310
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -424,10 +424,10 @@ func (p *ProvisionAction) Run(ctx context.Context) (*actions.ActionResult, error | |
| }, nil | ||
| } | ||
|
|
||
| skipped := deployResult.SkippedReason == provisioning.DeploymentStateSkipped | ||
| skipped := isDeploymentSkipped(deployResult) | ||
| allSkipped = allSkipped && skipped | ||
| if skipped { | ||
| // Simply continue here; message is printed in the provider implementation | ||
| // Simply continue here; message is printed in the provider implementation. | ||
| continue | ||
| } | ||
|
|
||
|
|
@@ -550,3 +550,12 @@ func GetCmdProvisionHelpDescription(c *cobra.Command) string { | |
| " When omitted, provisions resources for all layers defined in the project."), | ||
| }) | ||
| } | ||
|
|
||
| // isDeploymentSkipped returns true if the deployment was skipped and the caller | ||
| // should not access deployResult.Deployment (which may be nil). | ||
| // A deployment is considered skipped when SkippedReason is non-empty, which includes | ||
| // states such as DeploymentStateSkipped (no changes) and PreflightAbortedSkipped | ||
| // (user declined after preflight warnings). | ||
| func isDeploymentSkipped(deployResult *provisioning.DeployResult) bool { | ||
|
Member
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. Adding a very specific check in the caller seems very tactical, and I'm wondering what we could do differently to codify or change the upstream behavior in @vhvb1989 if you have any thoughts here.
Member
Author
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. Great point — treating preflight abort as "skipped" is the root of the problem, not just the nil deref. The deeper issue: When a user declines preflight during Investigation summary:
Proposed options — would love your input on which direction: Option A: Provider returns error upstream Option B: Provision layer distinguishes abort from skip Option C: Manager returns error My lean is Option A — the provider knows the user declined, so it should signal that as an error at the source. The |
||
| return deployResult != nil && deployResult.SkippedReason != "" | ||
| } | ||
|
Comment on lines
+559
to
+561
Member
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. The fix is good to mitigate the bug, but not as a the right approach we want here. The root issue is that selecting **
** The right fix is to ensure azd stops the process, either azd up or azd provision
Member
Author
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. Agreed 100% — the right fix is to stop the process completely, not skip. I posted a detailed analysis on @weikanglim's thread above with 3 options for how to propagate the abort. The TL;DR: the provider should return an error (wrapping the existing Will update the PR once we align on the approach. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| package cmd | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/azure/azure-dev/cli/azd/pkg/infra/provisioning" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // TestIsDeploymentSkipped_AllSkipReasons verifies that isDeploymentSkipped | ||
| // correctly identifies ALL skip reasons, not just DeploymentStateSkipped. | ||
| // | ||
| // Regression test for https://github.com/Azure/azure-dev/issues/7305: | ||
| // When the user declines preflight validation warnings, Deploy returns | ||
| // PreflightAbortedSkipped with a nil Deployment. If this skip reason is not | ||
| // detected, the caller dereferences nil Deployment.Outputs and panics. | ||
| func TestIsDeploymentSkipped_AllSkipReasons(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| result *provisioning.DeployResult | ||
| expectSkipped bool | ||
| nilDeployment bool | ||
| }{ | ||
|
jongio marked this conversation as resolved.
|
||
| { | ||
| name: "DeploymentStateSkipped", | ||
| result: &provisioning.DeployResult{ | ||
| SkippedReason: provisioning.DeploymentStateSkipped, | ||
| }, | ||
| expectSkipped: true, | ||
| nilDeployment: true, | ||
|
jongio marked this conversation as resolved.
|
||
| }, | ||
| { | ||
| // This is the regression case from issue #7305. | ||
| // Before the fix, this was NOT detected as skipped, causing a nil | ||
| // pointer dereference when accessing Deployment.Outputs. | ||
| name: "PreflightAbortedSkipped", | ||
| result: &provisioning.DeployResult{ | ||
| SkippedReason: provisioning.PreflightAbortedSkipped, | ||
| }, | ||
| expectSkipped: true, | ||
| nilDeployment: true, | ||
| }, | ||
| { | ||
| name: "NotSkipped_WithDeployment", | ||
| result: &provisioning.DeployResult{ | ||
| Deployment: &provisioning.Deployment{ | ||
| Outputs: map[string]provisioning.OutputParameter{}, | ||
| }, | ||
| }, | ||
| expectSkipped: false, | ||
| nilDeployment: false, | ||
| }, | ||
| { | ||
| name: "NilDeployResult", | ||
| result: nil, | ||
| expectSkipped: false, | ||
| nilDeployment: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| skipped := isDeploymentSkipped(tt.result) | ||
| require.Equal(t, tt.expectSkipped, skipped, | ||
| "isDeploymentSkipped returned unexpected value") | ||
|
|
||
| if tt.result == nil { | ||
| return | ||
| } | ||
|
|
||
| // Verify the Deployment nil/non-nil state matches expectations. | ||
| // This is important: the bug in #7305 was caused by accessing | ||
| // Deployment.Outputs when Deployment was nil. | ||
| if tt.nilDeployment { | ||
| require.Nil(t, tt.result.Deployment, | ||
| "when skipped, Deployment may be nil (accessing it would panic)") | ||
| } else { | ||
| require.NotNil(t, tt.result.Deployment, | ||
| "when not skipped, Deployment must not be nil (callers access Deployment.Outputs)") | ||
| } | ||
| }) | ||
| } | ||
| } | ||

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.
allSkippedis updated for any non-emptySkippedReason, includingPreflightAbortedSkipped. If the user declines preflight warnings (common single-layer case),allSkippedremains true and the function returns "There are no changes to provision...", which is misleading because the run was user-aborted, not a no-op. Consider handlingPreflightAbortedSkippedseparately (e.g., return an ActionResult indicating provisioning was aborted/canceled, and/or avoid folding it into the "no changes" allSkipped path).