Skip to content

Commit d565c8a

Browse files
fix: address Copilot PR review feedback
Resolves 12 inline comments from Copilot reviewer on PR #1009: setup_local_dev.sh: * Add Bash 4+ requirement check at top (macOS ships 3.2 — `declare -A` is not available there). Fail with actionable Homebrew install hint. * Remove silent fallback to Python <3.12 in interpreter detection; rely on check_prerequisites to fail loudly when 3.12+ is missing. * Make generated .vscode/settings.json `python.defaultInterpreterPath` OS-aware (bin/python on Linux/macOS, Scripts/python.exe on Windows). setup_local_dev.ps1: * Add `#Requires -Version 7.0` plus runtime guard — `??` and `Out-File -Encoding utf8NoBOM` aren't supported on Windows PowerShell 5.1. * Remember the detected Python invocation (e.g., `py -3.12`) and reuse it when creating the frontend venv, instead of unconditionally calling `python -m venv` (which fails when `python` isn't on PATH). deploy_to_azure.sh: * Fix az_retry comment to say "up to 4 attempts" to match the loop bound. * Correct step header in update_azure_resources from "Step 7" to "Step 8" so log output matches docs/section structure. * Add `_has_az_executable` helper using `type -P az` so the prereq check isn't satisfied by the `az()` wrapper function defined earlier in the script when the real Azure CLI isn't installed. deploy_to_azure.ps1: * In Configure-AcrOnResources, capture exit code of BOTH frontend `az webapp config` calls. Previously only the second was checked, so a failure to set DOCKER_REGISTRY_SERVER_URL could be silently masked by a successful acrUseManagedIdentityCreds update. docs: * DeployLocalChanges.md — add Step 1b (Azure roles & permissions check) to the "What It Does (in order)" list. * AutomatedLocalSetup.md — add Step 2b (Azure roles & permissions check) to the "What It Does (in order)" list. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 7efaf7c commit d565c8a

6 files changed

Lines changed: 79 additions & 23 deletions

File tree

docs/AutomatedLocalSetup.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ az login
3131

3232
1. **Checks prerequisites** — Python 3.12+, Node.js, npm, uv, Azure CLI, Git
3333
2. **Azure authentication** — logs you in if needed, confirms the active subscription
34+
2b. **Checks Azure roles & permissions** — verifies you have role-assignment permission (Owner / User Access Administrator / RBAC Administrator) before attempting RBAC step (non-fatal warning)
3435
3. **Fetches Azure configuration** — reads deployment outputs or queries resources individually to build `src/backend/.env`
3536
4. **Assigns RBAC roles** — grants your user account the roles needed to run the app locally:
3637
- Cosmos DB Built-in Data Contributor

docs/DeployLocalChanges.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ az login
2626
## What It Does (in order)
2727

2828
1. **Checks prerequisites** — Azure CLI and Docker (both required)
29+
1b. **Checks Azure roles & permissions** — verifies you have Contributor + role-assignment permission on the resource group (non-fatal warning)
2930
2. **Discovers Azure resources** — finds the backend/MCP Container Apps and frontend App Service in your resource group
3031
3. **Resolves ACR** — lists ACRs in the resource group and asks which one to use; prompts to create a new one if needed
3132
4. **Determines services** — deploys all services by default, or only the ones you specify with `--services`

infra/scripts/deploy_to_azure.ps1

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -646,12 +646,17 @@ function Configure-AcrOnResources {
646646
if ($DryRun) { Write-LogInfo "[DRY RUN] Would update frontend App Service registry config" }
647647
else {
648648
Write-LogInfo "Updating frontend App Service registry config..."
649+
# Capture exit codes from BOTH commands — without this, a failure in the
650+
# first call (DOCKER_REGISTRY_SERVER_URL) would be masked if the second
651+
# call succeeds, leaving the Web App with a half-configured registry.
649652
az webapp config appsettings set --name $script:FrontendApp --resource-group $ResourceGroup `
650653
--settings DOCKER_REGISTRY_SERVER_URL="https://$($script:AcrLoginServer)" --output none
654+
$appsettingsRc = $LASTEXITCODE
651655
az webapp config set --name $script:FrontendApp --resource-group $ResourceGroup `
652656
--generic-configurations '{\"acrUseManagedIdentityCreds\": true}' --output none
653-
if ($LASTEXITCODE -ne 0) {
654-
Write-LogError "Frontend registry config FAILED — image pull may fail."
657+
$configRc = $LASTEXITCODE
658+
if ($appsettingsRc -ne 0 -or $configRc -ne 0) {
659+
Write-LogError "Frontend registry config FAILED (appsettings rc=$appsettingsRc, config rc=$configRc) — image pull may fail."
655660
} else {
656661
Write-LogSuccess "Frontend registry config updated"
657662
}

infra/scripts/deploy_to_azure.sh

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,11 @@ set -euo pipefail
2525
# See: https://github.com/Azure/azure-cli/issues/13009
2626
az() { MSYS_NO_PATHCONV=1 MSYS2_ARG_CONV_EXCL="*" command az "$@"; }
2727

28+
# Probe for the real az executable that ignores the wrapper function above.
29+
# Used in check_prerequisites so an uninstalled Azure CLI doesn't pass the check
30+
# merely because `command -v az` matches our shell function.
31+
_has_az_executable() { type -P az >/dev/null 2>&1; }
32+
2833
# Convert a path to Windows-native format for tools like docker.exe that need it.
2934
# On non-MSYS systems (Linux/macOS) this is a no-op.
3035
_winpath() {
@@ -73,7 +78,7 @@ log_warn() { echo -e "${YELLOW}[!]${NC} $*"; }
7378
log_error() { echo -e "${RED}[✗]${NC} $*"; }
7479
log_step() { echo -e "\n${CYAN}━━━ $* ━━━${NC}\n"; }
7580

76-
# Retry an az command up to 3 times on transient network errors
81+
# Retry an az command up to 4 attempts on transient network/operation-in-progress errors
7782
az_retry() {
7883
local attempt=1 out rc delay
7984
while [[ $attempt -le 4 ]]; do
@@ -160,7 +165,7 @@ check_prerequisites() {
160165

161166
local missing=()
162167

163-
if command -v az &>/dev/null; then
168+
if _has_az_executable; then
164169
log_success "Azure CLI found"
165170
else
166171
missing+=("azure-cli")
@@ -808,7 +813,7 @@ configure_acr_on_resources() {
808813
# ==============================================================================
809814

810815
update_azure_resources() {
811-
log_step "Step 7: Updating Azure Resources"
816+
log_step "Step 8: Updating Azure Resources"
812817

813818
if [[ "$BUILD_ONLY" == true ]]; then
814819
return

infra/scripts/setup_local_dev.ps1

Lines changed: 37 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
# .\setup_local_dev.ps1 -ResourceGroup "rg-macae-dev" -SkipPrereqs
1414
# ==============================================================================
1515

16+
#Requires -Version 7.0
17+
1618
param(
1719
[string]$ResourceGroup = "",
1820
[string]$Subscription = "",
@@ -22,6 +24,16 @@ param(
2224

2325
$ErrorActionPreference = "Stop"
2426

27+
# PowerShell 7+ is required because this script uses the null-coalescing operator (`??`)
28+
# and `Out-File -Encoding utf8NoBOM`, neither of which exist in Windows PowerShell 5.1.
29+
# `#Requires -Version 7.0` aborts with a clear error on older hosts before parsing fails.
30+
if ($PSVersionTable.PSVersion.Major -lt 7) {
31+
Write-Host "[ERROR] This script requires PowerShell 7 or newer. Detected: $($PSVersionTable.PSVersion)" -ForegroundColor Red
32+
Write-Host " Install from https://aka.ms/powershell or 'winget install Microsoft.PowerShell'" -ForegroundColor Red
33+
Write-Host " Then re-run with 'pwsh.exe' instead of 'powershell.exe'." -ForegroundColor Red
34+
exit 1
35+
}
36+
2537
# Resolve repo root (script lives in infra/scripts/)
2638
$ScriptDir = $PSScriptRoot
2739
if (-not $ScriptDir) { $ScriptDir = Get-Location }
@@ -67,21 +79,32 @@ function Check-Prerequisites {
6779
$missing = @()
6880

6981
# Python 3.12+
82+
# Detect a Python 3.12+ interpreter and remember the exact invocation so later
83+
# steps (venv creation) use the same interpreter. Without this, `py -3.12` may pass
84+
# prereqs while a later `python -m venv` call fails because `python` isn't on PATH.
85+
$script:PythonInvocation = $null
7086
if (Test-CommandExists "py") {
7187
$pyVersion = & py -3.12 --version 2>$null
7288
if ($pyVersion) {
7389
Write-LogSuccess "Python 3.12 found: $pyVersion"
74-
} else {
75-
$missing += "Python.Python.3.12"
90+
$script:PythonInvocation = @("py", "-3.12")
7691
}
77-
} elseif (Test-CommandExists "python") {
92+
}
93+
if (-not $script:PythonInvocation -and (Test-CommandExists "python")) {
7894
$pyVersion = & python --version 2>$null
7995
if ($pyVersion -match "3\.1[2-9]") {
8096
Write-LogSuccess "Python found: $pyVersion"
81-
} else {
82-
$missing += "Python.Python.3.12"
97+
$script:PythonInvocation = @("python")
8398
}
84-
} else {
99+
}
100+
if (-not $script:PythonInvocation -and (Test-CommandExists "python3")) {
101+
$pyVersion = & python3 --version 2>$null
102+
if ($pyVersion -match "3\.1[2-9]") {
103+
Write-LogSuccess "Python found: $pyVersion"
104+
$script:PythonInvocation = @("python3")
105+
}
106+
}
107+
if (-not $script:PythonInvocation) {
85108
$missing += "Python.Python.3.12"
86109
}
87110

@@ -840,7 +863,14 @@ function Setup-Frontend {
840863
if (-not (Test-Path ".venv\Scripts\activate")) {
841864
if (Test-Path ".venv") { Write-LogWarn "Existing .venv is incomplete (no activate script), recreating..." }
842865
Write-LogInfo "Creating Python virtual environment..."
843-
python -m venv --clear .venv
866+
# Use the same interpreter Check-Prerequisites detected (handles 'py -3.12'
867+
# on Windows machines where 'python' isn't on PATH).
868+
$pyExe = if ($script:PythonInvocation) { $script:PythonInvocation[0] } else { "python" }
869+
$pyArgs = @()
870+
if ($script:PythonInvocation -and $script:PythonInvocation.Count -gt 1) {
871+
$pyArgs = $script:PythonInvocation[1..($script:PythonInvocation.Count - 1)]
872+
}
873+
& $pyExe @pyArgs -m venv --clear .venv
844874
} else {
845875
Write-LogInfo "Python virtual environment already exists"
846876
}

infra/scripts/setup_local_dev.sh

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,19 @@
1717

1818
set -euo pipefail
1919

20+
# ==============================================================================
21+
# Bash version check (associative arrays require Bash 4+; macOS ships Bash 3.2)
22+
# ==============================================================================
23+
if [[ -z "${BASH_VERSION:-}" ]] || [[ "${BASH_VERSINFO[0]:-0}" -lt 4 ]]; then
24+
echo "[ERROR] This script requires Bash 4 or newer (associative arrays)." >&2
25+
echo " Detected: ${BASH_VERSION:-unknown}" >&2
26+
echo " macOS ships with Bash 3.2 by default — install Bash 4+ via Homebrew:" >&2
27+
echo " brew install bash" >&2
28+
echo " Then re-run with the new interpreter, e.g.:" >&2
29+
echo " /opt/homebrew/bin/bash infra/scripts/setup_local_dev.sh ..." >&2
30+
exit 1
31+
fi
32+
2033
# ==============================================================================
2134
# Paths
2235
# ==============================================================================
@@ -50,15 +63,8 @@ for _cmd in python3.12 python3 python py; do
5063
fi
5164
fi
5265
done
53-
# Fallback: use whatever python3/python is available even if version check failed
54-
if [[ -z "$PYTHON_CMD" ]]; then
55-
for _cmd in python3 python py; do
56-
if command -v "$_cmd" &>/dev/null; then
57-
PYTHON_CMD="$_cmd"
58-
break
59-
fi
60-
done
61-
fi
66+
# No silent fallback to <3.12 — later steps (uv sync --python 3.12, venv) require 3.12+.
67+
# If nothing suitable is found, leave PYTHON_CMD empty; check_prerequisites will fail loudly.
6268

6369
# ==============================================================================
6470
# Colors
@@ -979,9 +985,17 @@ EXTEOF
979985

980986
local settings_file="$vscode_dir/settings.json"
981987
if [[ ! -f "$settings_file" ]]; then
982-
cat > "$settings_file" << 'SETEOF'
988+
# Choose an OS-appropriate interpreter sub-path (Windows uses Scripts\python.exe,
989+
# Linux/macOS use bin/python). Detect by checking what activate script exists.
990+
local interp_path
991+
if [[ -f "$BACKEND_DIR/.venv/Scripts/python.exe" ]] || [[ "$OSTYPE" == "msys" || "$OSTYPE" == "cygwin" || "$OSTYPE" == "win32" ]]; then
992+
interp_path='${workspaceFolder}/src/backend/.venv/Scripts/python.exe'
993+
else
994+
interp_path='${workspaceFolder}/src/backend/.venv/bin/python'
995+
fi
996+
cat > "$settings_file" << SETEOF
983997
{
984-
"python.defaultInterpreterPath": "${workspaceFolder}/src/backend/.venv/Scripts/python.exe",
998+
"python.defaultInterpreterPath": "$interp_path",
985999
"python.terminal.activateEnvironment": true,
9861000
"python.linting.enabled": true,
9871001
"python.formatting.provider": "black",

0 commit comments

Comments
 (0)