fix: ACR build and push script failing - #692
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors the ACR remote build/push automation for Bash and PowerShell by staging a temporary Docker build context (intended to limit what gets sent to ACR), and updates ContentProcessorWeb’s .dockerignore to better exclude common local artifacts from build contexts.
Changes:
- Refactors
acr_build_push.shto stage a temporary build context directory per image before runningaz acr build. - Refactors
acr_build_push.ps1similarly, improving path handling and ensuring staging cleanup viatry/finally. - Updates
src/ContentProcessorWeb/.dockerignorewith additional exclusions and comment cleanup.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/ContentProcessorWeb/.dockerignore | Adds additional ignore patterns intended to keep common frontend/build artifacts out of ACR build contexts. |
| infra/scripts/acr_build_push.sh | Introduces staging-directory build contexts for ACR builds and related refactoring. |
| infra/scripts/acr_build_push.ps1 | Introduces staging-directory build contexts for ACR builds, with improved path handling and cleanup. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
infra/scripts/acr_build_push.sh:123
- The staging directory cleanup only happens after
az acr buildreturns; withset -e, any failure during the copy loop (e.g.cp/mkdirfailing) will exit the script before therm -rfruns, leaving temp directories behind. Consider wrapping the function body in atrap/finally-style cleanup that runs on any exit path forbuild_image.
staging_dir="$(mktemp -d)"
while IFS= read -r -d '' source_file; do
staged_file="${source_file#"$context_path"/}"
mkdir -p "$staging_dir/$(dirname "$staged_file")"
cp -a "$REPO_ROOT/$source_file" "$staging_dir/$staged_file"
infra/scripts/acr_build_push.sh:2
- The script has a blank first line, so the
#!/bin/bashshebang is not on line 1. This means running it as an executable (e.g../infra/scripts/acr_build_push.sh) will not invoke bash via the shebang and can fail with an exec format error; move the shebang to the very first line (remove the leading blank line).
#!/bin/bash
infra/scripts/acr_build_push.sh:130
git ls-filesis invoked with--others --exclude-standard, which will include untracked (but not ignored) files in the build context. This contradicts the PR goal of staging only tracked/source-controlled files and can accidentally ship local working-copy files; drop--others(and--cachedis redundant since tracked files are the default).
git -C "$REPO_ROOT" ls-files \
--cached \
--others \
--exclude-standard \
-z \
-- "$context_path"
infra/scripts/acr_build_push.ps1:157
git ls-filesis invoked with--others --exclude-standard, which will include untracked (but not ignored) files in the staged Docker build context. If the intent is “tracked files only” (per PR description), remove--others(and--cachedis redundant because tracked files are the default output ofgit ls-files).
$SourceFiles = git -C $RepoRoot ls-files `
--cached `
--others `
--exclude-standard `
-- $ContextPath
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
infra/scripts/acr_build_push.sh:2
- The script has a blank line before the shebang. For executable scripts, the shebang must be on the very first line (otherwise the OS may not invoke bash when run as
./acr_build_push.sh). Remove the leading blank line so#!/bin/bashis line 1.
#!/bin/bash
infra/scripts/acr_build_push.sh:130
git ls-filesis invoked with--others, which includes untracked files in the staging build context. This contradicts the stated goal of including only tracked source files and can unintentionally ship local/uncommitted artifacts (potentially sensitive) into the image. Consider removing--others(or otherwise constraining the file list) if the intent is truly tracked-only.
git -C "$REPO_ROOT" ls-files \
--cached \
--others \
--exclude-standard \
-z \
-- "$context_path"
infra/scripts/acr_build_push.ps1:157
git ls-filesis invoked with--others, which includes untracked files in the staging build context. This contradicts the stated goal of including only tracked source files and can unintentionally ship local/uncommitted artifacts (potentially sensitive) into the image. Consider removing--others(or otherwise constraining the file list) if the intent is truly tracked-only.
$SourceFiles = git -C $RepoRoot ls-files `
--cached `
--others `
--exclude-standard `
-- $ContextPath
src/ContentProcessorWeb/.dockerignore:33
- The new comment says these are "explicit root-level exclusions", but patterns like
node_modules/build/coverageare not root-anchored in .dockerignore and will match at any level. Alsonode_modulesis already covered by the existing**/node_modulesentry, so this is currently redundant. Either anchor these patterns with a leading/(if you truly mean root-only) or adjust the comment/remove duplicates.
# Azure CLI context packaging requires explicit root-level exclusions.
node_modules
build
coverage
Purpose
This pull request refactors and improves the Azure Container Registry (ACR) build and push scripts for both PowerShell and Bash, and updates the
.dockerignorefile forContentProcessorWeb. The main goals are to ensure only tracked source files are included in Docker build contexts, improve cross-platform compatibility, and streamline the handling of temporary build directories.Build context handling and script improvements:
acr_build_push.ps1andacr_build_push.shscripts to create a temporary staging directory for each image build, copying only files tracked by Git (usinggit ls-files). This ensures that only relevant, version-controlled files are included in the Docker build context, reducing image size and avoiding accidental inclusion of local or untracked files. [1] [2].Pathproperty for consistency and cross-platform compatibility.Join-Pathand string interpolation for file paths in the PowerShell script, improving readability and maintainability..dockerignore updates for ContentProcessorWeb:
node_modules,build,coverage,.cache,.parcel-cache,.pnpm-store,.pnpm-cache,.pnpm). This aligns with the new build context logic and prevents unnecessary files from being included in Docker images. [1] [2]Does this introduce a breaking change?
Golden Path Validation
Deployment Validation
What to Check
Verify that the following are valid
Other Information