Expose ARM deployment IDs via AZD_DEPLOYMENT_ID_FILE during provision (NDJSON)#8047
Open
Expose ARM deployment IDs via AZD_DEPLOYMENT_ID_FILE during provision (NDJSON)#8047
Conversation
Agent-Logs-Url: https://github.com/Azure/azure-dev/sessions/fe28b385-10ed-4456-adc7-6d188df69534 Co-authored-by: bwateratmsft <36966225+bwateratmsft@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Add machine-readable output for azd provision command
Expose ARM deployment ID via AZD_DEPLOYMENT_ID_FILE during provision
May 5, 2026
Contributor
|
Marking as ready so I can get a build to test with. |
bwateratmsft
requested review from
JeffreyCA,
hemarina,
jongio,
rajeshkamal5050,
tg-msft,
vhvb1989,
wbreza and
weikanglim
as code owners
📋 Prioritization NoteThanks for the contribution! The linked issue isn't in the current milestone yet. |
Contributor
|
@copilot apply changes based on the comments in this thread |
…concurrent writes, doc clarification Agent-Logs-Url: https://github.com/Azure/azure-dev/sessions/00137c09-6945-426c-a415-e61a29b7fca2 Co-authored-by: bwateratmsft <36966225+bwateratmsft@users.noreply.github.com>
Contributor
Author
Applied in c7fd541:
|
… contract Agent-Logs-Url: https://github.com/Azure/azure-dev/sessions/4b1aa11f-0d0d-458e-8eef-f10c012a7aa5 Co-authored-by: bwateratmsft <36966225+bwateratmsft@users.noreply.github.com>
Copilot
AI
changed the title
Expose ARM deployment ID via AZD_DEPLOYMENT_ID_FILE during provision
Expose ARM deployment IDs via AZD_DEPLOYMENT_ID_FILE during provision (NDJSON)
May 7, 2026
Contributor
|
@copilot please fix the build. cspell failed on the word "subtests" in the test file. |
Agent-Logs-Url: https://github.com/Azure/azure-dev/sessions/f9e2b299-0376-4e7c-9434-e100ad6e2f55 Co-authored-by: bwateratmsft <36966225+bwateratmsft@users.noreply.github.com>
Contributor
Author
wbreza
approved these changes
May 7, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Re-Review — PR #8047 ✅
All findings from the initial review have been addressed in commit 5 (9f93223):
| Finding | Status |
|---|---|
🔴 P1: Missing docs/reference/environment-variables.md entry |
✅ Fixed — entry added with link to full contract |
| 🟡 P2: Missing test for unsupported deployment type | ✅ Fixed — UnsupportedType_ReturnsError test added |
| 🟡 P2: Missing test for path-is-a-directory edge case | ✅ Fixed — PathIsDirectory_DoesNotPanic test added |
| 🟡 P2: Error scenarios under-documented | ✅ Fixed — docs now cover missing dir, unwritable path, directory target, Windows file-handle |
| 🟡 P2: Symlink target not validated |
Implementation is solid. Approving.
wbreza
reviewed
May 7, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Re-Review — PR #8047 (NDJSON Changes)
Commits 6–9 switch from single-JSON-overwrite to NDJSON append. The multi-layer design is a good direction, but the truncation logic has a correctness bug that should be fixed before merge.
🔴 P1 — sync.Once truncation bug: failed truncation silently ignored on subsequent calls
writeDeploymentIdFile declares truncateErr as a local variable each invocation. If os.Truncate fails on the first call, sync.Once is consumed. On the second call, truncateErr is a fresh nil, the Do() is a no-op, and the function proceeds to append to a file that was never truncated — mixing stale data from previous runs with new entries.
var truncateErr error // fresh local, zero-value nil
deploymentIdFileTruncated.Do(func() { // no-op after first call
truncateErr = os.Truncate(path, 0)
})
if truncateErr != nil { return } // second caller passes (nil)Fix: Move truncateErr to package scope so the error persists across calls:
var deploymentIdFileTruncateErr error
deploymentIdFileTruncated.Do(func() {
deploymentIdFileTruncateErr = os.Truncate(path, 0)
if os.IsNotExist(deploymentIdFileTruncateErr) {
deploymentIdFileTruncateErr = nil
}
})
if deploymentIdFileTruncateErr != nil { ... }Alternatively, replace sync.Once with a simple bool + the existing mutex (already held at the point of truncation).
🟡 P2 — resetDeploymentIdFileTruncation() not mutex-protected
The reset function reassigns the sync.Once without holding deploymentIdFileMu. Tests are safe today (sequential subtests, no t.Parallel()), but this is a latent race. Fix: acquire the mutex before resetting, and reset the truncation error too.
🟡 P2 — Missing regression test for truncation-failure path
No test exercises: truncation fails then second call appends to untruncated file. This is exactly the P1 scenario. Please add a test alongside the fix.
🟡 P2 — Atomicity regression from temp+rename to O_APPEND
The old temp+rename was fully atomic for external readers. The new O_APPEND design relies on OS-level write atomicity, which is weaker on Windows. Low practical risk for ~150-byte lines, but worth a code comment noting the trade-off and why it is acceptable for this use case.
⚪ P3 — Minor
- Concurrent test could verify exact layer counts (4 sub + 4 rg), not just total
readNDJSONLinessilently skips empty lines — consider failing on unexpected structure
✅ Confirmed correct
NDJSON line format, multi-layer append logic, call site passing p.layer, documentation in both locations, cspell fix.
Contributor
|
@copilot please fix the issues described in #8047 (review) |
Agent-Logs-Url: https://github.com/Azure/azure-dev/sessions/0e4c7804-120c-41a9-baa8-f766d476b3bb Co-authored-by: bwateratmsft <36966225+bwateratmsft@users.noreply.github.com>
Contributor
Author
Addressed in d607344:
|
wbreza
approved these changes
May 7, 2026
Contributor
wbreza
left a comment
wbreza
left a comment
There was a problem hiding this comment.
Re-Review — PR #8047 (Truncation Fix)
Commit 10 (d607344) fixes all findings from the NDJSON review:
| Finding | Status |
|---|---|
P1: sync.Once truncation bug |
Fixed — replaced with bool + package-scoped error, both guarded by mutex |
P2: resetDeploymentIdFileTruncation() not mutex-protected |
Fixed — now acquires deploymentIdFileMu |
| P2: Missing regression test for truncation failure | Fixed — TruncationFailure_BlocksSubsequentAppends added |
| P2: Atomicity regression (O_APPEND vs temp+rename) | Accepted design trade-off for multi-layer append |
Clean fix. Approving.
Contributor
|
@copilot please fix the lint errors...c'mon... |
Agent-Logs-Url: https://github.com/Azure/azure-dev/sessions/2be8c9db-cb17-483f-9724-d866c7ef0bc5 Co-authored-by: bwateratmsft <36966225+bwateratmsft@users.noreply.github.com>
Contributor
Author
Fixed in e635e2e — gosec G706 was flagging the multi-line |
jongio
reviewed
May 7, 2026
Member
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
The NDJSON evolution since my last review is well done. The truncate-on-first-write pattern with persisted error state is a good defensive choice - it ensures stale content from a previous run can't mix with current-run lines even when the first layer's truncation fails.
Tests cover the important edge cases (truncation failure blocking appends, directory-as-path, concurrent writers). @wbreza's findings about the original sync.Once issue are fully addressed.
No new issues. Ready from my side once CI is green.
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
|
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.
layerfieldresetDeploymentIdFileTruncationTruncationFailure_BlocksSubsequentAppendsregression test//nolintdirective above the multi-linelog.Printf)