corrected devcontainer to work on arm64 computers#28939
Conversation
| Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
There was a problem hiding this comment.
Pull request overview
This PR aims to improve the devcontainer configuration for Azure PowerShell development by creating a custom Dockerfile and moving setup commands from inline to a dedicated post-create script. However, there is a critical issue: the PR title claims to enable ARM64 support, but the Dockerfile explicitly forces AMD64 architecture, which will cause emulation overhead on ARM64 machines rather than native support.
Key Changes:
- Introduces custom Dockerfile with explicit platform specification
- Moves setup commands to a dedicated
post-create.shscript - Adds additional PowerShell module installations (Az.Accounts)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| .devcontainer/Dockerfile | Creates custom Dockerfile that forces AMD64 platform (contradicts PR goal of ARM64 support) |
| .devcontainer/post-create.sh | Extracts setup commands into dedicated script; installs npm, AutoRest, platyPS, and Az.Accounts |
| .devcontainer/devcontainer.json | Updates to use custom Dockerfile and post-create script instead of inline command |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@dolauli , Isolated change for devcontainer, as suggesed in previous PR |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
.devcontainer/post-create.sh
Outdated
| @@ -0,0 +1,18 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
The post-create.sh script should be marked as executable in the git repository. While devcontainer implementations typically handle this automatically, it's best practice to ensure the script has execute permissions committed to the repository.
You can verify and set this with:
chmod +x .devcontainer/post-create.sh
git add .devcontainer/post-create.sh
This ensures the script will be executable when cloned or when Docker builds the container.
.devcontainer/post-create.sh
Outdated
| echo "� Updating npm..." | ||
| npm update -g npm | ||
|
|
||
| echo "�📦 Installing AutoRest..." |
There was a problem hiding this comment.
The echo statements on lines 4 and 7 contain malformed emoji characters that appear as "�" and "�📦". This suggests an encoding issue during file creation or editing.
These should be corrected to use proper UTF-8 encoded emojis or replaced with plain text for better compatibility. For example:
- Line 4: "🔄 Updating npm..." or "Updating npm..."
- Line 7: "📦 Installing AutoRest..." or "Installing AutoRest..."
| echo "� Updating npm..." | |
| npm update -g npm | |
| echo "�📦 Installing AutoRest..." | |
| echo "🔄 Updating npm..." | |
| npm update -g npm | |
| echo "📦 Installing AutoRest..." |
| # Set working directory | ||
| WORKDIR /workspace | ||
|
|
There was a problem hiding this comment.
The working directory is set to /workspace, but the devcontainer.json mounts the workspace at /workspaces/${localWorkspaceFolderBasename} (note the plural "workspaces"). This mismatch means the WORKDIR directive has no practical effect since the actual workspace will be mounted at a different location.
Consider either:
- Removing this WORKDIR directive as it's not needed (the devcontainer will automatically set the working directory to the mounted workspace), or
- Changing it to match the actual mount point if you need to ensure consistency
| # Set working directory | |
| WORKDIR /workspace |
|
|
||
| # Set default user to root for pwsh compatibility | ||
| USER root |
There was a problem hiding this comment.
The USER root directive is redundant here because the base image mcr.microsoft.com/devcontainers/dotnet:1-8.0 already runs as a non-root user by default, and the devcontainer.json explicitly sets "remoteUser": "root" on line 47, which will override any USER directive in the Dockerfile.
This line can be safely removed as it has no effect given the devcontainer.json configuration.
| # Set default user to root for pwsh compatibility | |
| USER root |
.devcontainer/post-create.sh
Outdated
| pwsh -c "Install-Module -Name platyPS -Force -Scope CurrentUser" | ||
|
|
||
| echo "🔐 Installing Az.Accounts for authentication..." | ||
| pwsh -c "Install-Module -Name Az.Accounts -Force -Scope CurrentUser" | ||
|
|
||
| echo "🧪 Installing PSScriptAnalyzer for static analysis..." | ||
| pwsh -c "Install-Module -Name PSScriptAnalyzer -Force -Scope CurrentUser" |
There was a problem hiding this comment.
The script installs PowerShell modules with -Scope CurrentUser, but since the devcontainer runs as root user (as configured in devcontainer.json line 47), these modules will be installed in the root user's profile. This is appropriate for a containerized development environment, but note that if the remoteUser configuration changes in the future, these modules would need to be reinstalled.
This is acceptable for the current configuration, but consider adding a comment in the script explaining this design choice for future maintainers.
.devcontainer/post-create.sh
Outdated
|
|
||
| echo "🧪 Installing PSScriptAnalyzer for static analysis..." | ||
| pwsh -c "Install-Module -Name PSScriptAnalyzer -Force -Scope CurrentUser" | ||
|
|
There was a problem hiding this comment.
The script is missing the PowerShell execution policy configuration that is listed as a prerequisite in the repository's development documentation. While execution policies are primarily a Windows concern and Linux/macOS PowerShell has fewer restrictions by default, it's good practice to explicitly set this to ensure consistent behavior across environments.
Consider adding this line after the PSScriptAnalyzer installation:
echo "⚙️ Setting PowerShell execution policy..."
pwsh -c "Set-ExecutionPolicy -ExecutionPolicy Unrestricted -Scope CurrentUser -Force"
This aligns with the prerequisites documented in documentation/development-docs/azure-powershell-developer-guide.md lines 57-60.
| echo "⚙️ Setting PowerShell execution policy..." | |
| pwsh -c "Set-ExecutionPolicy -ExecutionPolicy Unrestricted -Scope CurrentUser -Force" |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| #!/usr/bin/env pwsh | ||
| #Requires -Version 7.0 | ||
|
|
||
| $ErrorActionPreference = "Stop" |
There was a problem hiding this comment.
The script uses Set-StrictMode is missing, which is a PowerShell best practice for catching errors early. According to the coding guidelines for PowerShell scripts in this repository, scripts should include Set-StrictMode -Version 2.0 (or higher) for better error catching.
Add Set-StrictMode -Version 2.0 after the ErrorActionPreference setting.
| $ErrorActionPreference = "Stop" | |
| $ErrorActionPreference = "Stop" | |
| Set-StrictMode -Version 2.0 |
| Write-Host "📦 Updating npm..." -ForegroundColor Cyan | ||
| npm update -g npm | ||
| if ($LASTEXITCODE -ne 0) { | ||
| throw "Failed to update npm" | ||
| } | ||
|
|
||
| Write-Host "🚀 Installing AutoRest..." -ForegroundColor Cyan | ||
| npm install -g autorest | ||
| if ($LASTEXITCODE -ne 0) { | ||
| throw "Failed to install AutoRest" |
There was a problem hiding this comment.
The script includes proper error handling with $ErrorActionPreference = "Stop", try-catch blocks for exit codes, and -ErrorAction Stop for PowerShell cmdlets. However, the npm update and npm install commands could fail in non-obvious ways. Consider adding informative error messages that include troubleshooting hints, such as checking network connectivity or npm installation.
For example, wrap the npm operations in a try-catch that provides more context about common failure scenarios.
| Write-Host "📦 Updating npm..." -ForegroundColor Cyan | |
| npm update -g npm | |
| if ($LASTEXITCODE -ne 0) { | |
| throw "Failed to update npm" | |
| } | |
| Write-Host "🚀 Installing AutoRest..." -ForegroundColor Cyan | |
| npm install -g autorest | |
| if ($LASTEXITCODE -ne 0) { | |
| throw "Failed to install AutoRest" | |
| try { | |
| Write-Host "📦 Updating npm..." -ForegroundColor Cyan | |
| npm update -g npm | |
| if ($LASTEXITCODE -ne 0) { | |
| throw "npm update failed with exit code $LASTEXITCODE" | |
| } | |
| Write-Host "🚀 Installing AutoRest..." -ForegroundColor Cyan | |
| npm install -g autorest | |
| if ($LASTEXITCODE -ne 0) { | |
| throw "AutoRest installation failed with exit code $LASTEXITCODE" | |
| } | |
| } catch { | |
| Write-Error "npm tooling setup failed: $($_.Exception.Message)" | |
| Write-Error ("Troubleshooting tips:`n" + | |
| " - Verify that Node.js and npm are installed and on the PATH (run 'npm --version').`n" + | |
| " - Check your network connectivity and any proxy/firewall settings that might block npm.`n" + | |
| " - If behind a proxy, configure npm to use it (e.g. 'npm config set proxy <url>').`n" + | |
| " - Retry this script after addressing the above issues.") | |
| exit 1 |
| "remoteUser": "root", | ||
|
|
||
| // Mount optimization for macOS | ||
| "workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/${localWorkspaceFolderBasename},type=bind,consistency=cached" |
There was a problem hiding this comment.
The workspaceMount uses the deprecated "consistency" option with value "cached". Docker Desktop for Mac deprecated the consistency flags (cached, delegated, consistent) in favor of automatic optimization. Modern versions of Docker Desktop ignore these flags, and they have no effect on Linux or Windows.
This option can be safely removed from the workspaceMount configuration as it no longer provides any benefit and may cause warnings in newer Docker versions.
| "workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/${localWorkspaceFolderBasename},type=bind,consistency=cached" | |
| "workspaceMount": "source=${localWorkspaceFolder},target=/workspaces/${localWorkspaceFolderBasename},type=bind" |
|
@wyunchi-ms , @dolauli , As requested, I have:
|
|
LGTM |
Description
Updated devcontainer configuration for Azure PowerShell development
The configuration includes:
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.mdand reviewed the following information:ChangeLog.mdfile(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.## Upcoming Releaseheader in the past tense. not the caseChangeLog.mdif no new release is required, such as fixing test case only.