azd down: warn before deleting resource groups not created by azd#7998
Open
rajeshkamal5050 wants to merge 1 commit intomainfrom
Open
azd down: warn before deleting resource groups not created by azd#7998rajeshkamal5050 wants to merge 1 commit intomainfrom
rajeshkamal5050 wants to merge 1 commit intomainfrom
Conversation
Fixes #4785 For RG-scoped deployments, azd down deletes the entire resource group even if it wasn't created by azd. This adds a check on the azd-env-name tag before deletion. If the tag is missing or doesn't match, the user gets a warning and confirmation prompt. - Matching tag: no change in behavior - Missing/mismatched tag: warning + confirm - --force: skips warning - --no-prompt: defaults to deny Note: does not support deleting individual resources while keeping the RG. This is consistent with sub-scoped deployments which also delete the entire resource group. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
rajeshkamal5050
requested review from
JeffreyCA,
hemarina,
jongio,
tg-msft,
vhvb1989,
wbreza and
weikanglim
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a safety check to azd down for resource-group-scoped deployments by verifying the target resource group is tagged with the current environment (azd-env-name) before deleting the entire RG, prompting for explicit confirmation when ownership can’t be verified.
Changes:
- Add
warnExternalResourceGroupto detect missing/mismatchedazd-env-nametags and require an extra confirmation before RG deletion. - Extend
azapi.ResourceGroupto includeTagsand updateGetResourceGroupto populate them. - Add unit tests covering matching tag, missing tag denial,
--forceskip, and API error fail-closed behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go |
Adds the external resource group warning/confirmation flow and integrates it into Destroy. |
cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go |
Adds unit tests validating the new warning/confirmation behavior. |
cli/azd/pkg/azapi/resource_service.go |
Adds Tags to ResourceGroup and maps ARM tag pointers to strings in GetResourceGroup. |
cli/azd/pkg/azapi/resource_service_coverage3_test.go |
Adds coverage tests verifying GetResourceGroup returns tags correctly (present vs absent). |
wbreza
requested changes
Apr 30, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Code Review — PR #7998
Verdict:
P1 — Should Fix Before Merge
1. Missing test: user confirms external RG deletion (happy path)
Tests cover force-skip, matching tag, denial, and API error — but never test the case where the user says "Yes" to deleting an external RG. This is the core feature path.
2. Missing test: mismatched tag value
Tests cover "no tag" and "matching tag" but NOT the case where azd-env-name exists with a different environment name (e.g., RG tagged azd-env-name=prod-env but current env is test-env). This is the most realistic real-world scenario.
3. Missing test: --no-prompt default-deny behavior
PR claims --no-prompt defaults to deny. This safety-critical behavior relies on DefaultValue: false in console.Confirm() but is untested. If the implicit behavior ever changes, external RGs could be silently deleted in CI/CD.
4. Confirm prompt lacks resource group context (bicep_provider.go)
The Y/N prompt says "Total resources to delete: N, are you sure you want to continue?" — generic and doesn't mention the resource group by name. After a warning about an external RG, the prompt should be explicit, e.g., "Do you still want to delete the entire resource group <name>?"
5. Confusing denial UX (bicep_provider.go)
On denial, code prints gray "Resource group was preserved..." then returns errors.New("user denied delete confirmation"). User sees "preserved" (positive) immediately followed by "ERROR" (negative). Consider removing the gray message and letting the error speak, or changing the flow so denial isn't an error.
P2 — Nice to Have
6. Error message in PR description doesn't match code
PR description shows "resource group deletion cancelled: resource group was not created by azd" but code returns "user denied delete confirmation". Consider updating either the PR description or the error message for consistency.
7. Consider ErrorWithSuggestion for user-fixable error (bicep_provider.go)
The denial error is user-fixable (re-run with --force) but provides no hint. Repo convention is ErrorWithSuggestion for actionable errors.
8. Error wrapping missing in Destroy() caller (bicep_provider.go)
return nil, err at the warnExternalResourceGroup call site has no context wrapping. Consider fmt.Errorf("checking external resource group: %w", err).
P3 — Informational
- Protobuf schema doesn't include
TagsonResourceGroupmessage — fine if gRPC callers don't need tags today; consider a follow-up. --forceskips all safety silently — correct behavior, consistent with flag semantics.- Tag spoofing is possible — acknowledged; this is a safety guard, not a security boundary.
✅ What's Good
- Fail-closed design: API errors → treat as external → warn. Correct and safe.
- No double-prompt: External RG confirmation skips the regular
promptDeletion(). Clean UX. - Backwards compatible: RGs with matching tags behave identically to before.
- Tag conversion is nil-safe and handles Azure SDK pointer types correctly.
- Tests follow repo conventions (
mocks.NewMockContext,t.Context(), subtests).
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
|
jongio
reviewed
Apr 30, 2026
Member
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Solid safety guard for RG-scoped azd down. The fail-closed design is the right call - API errors triggering the warning is better than silent deletion.
I agree with @wbreza's test gap findings (P1 #1-3). The missing "user confirms external RG" and "mismatched tag" cases are the most important - those are the core feature paths.
One additional observation:
The warnExternalResourceGroup return value (bool, error) conflates two concerns: "did the user already confirm?" and "should we skip the regular prompt?". When --force is true, it returns (true, nil) meaning "skip the regular prompt" - but the caller also skips promptDeletion which would have shown the resource list. This is fine today since promptDeletion also early-returns on --force, but the coupling is implicit. A comment on the return value contract (or renaming skipRegularPrompt to userAlreadyConfirmed) would make this clearer for future maintainers.
On P2 #8 from wbreza's review - the missing error wrapping at the call site is worth fixing. Right now a transient API failure surfaces as "error deleting Azure resources: user denied delete confirmation" with no hint that it was the tag-check API call that failed, not a user action.
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.
Fixes #4785
azd downwith RG-scoped deployments deletes the entire resource group even if azd didn't create it. This adds a tag check before deletion.Checks the
azd-env-nametag on the RG. If missing or mismatched, warns the user and asks for confirmation before proceeding.Unit tests added for the warning flow (force skip, matching tag, no-tag denial, API error fail-closed).
Testing
RG without azd-env-name tag (warns + user says No):
RG with azd-env-name tag (no warning, normal flow):
Note: does not support deleting individual resources while keeping the RG. Consistent with sub-scoped deployments.