Skip to content

Run samples on arm64 as well as amd64 in CI - #108

Draft
DrisDary wants to merge 3 commits into
mainfrom
feat/arm64-sample-coverage
Draft

Run samples on arm64 as well as amd64 in CI#108
DrisDary wants to merge 3 commits into
mainfrom
feat/arm64-sample-coverage

Conversation

@DrisDary

Copy link
Copy Markdown
Contributor

The Samples CI only ever ran on amd64, so nothing caught the Azure
Functions arm64 breakage that a reviewer hit on Apple Silicon (fixed in
localstack-pro#8102). Add an architecture axis to the test matrix.

Not every sample can run natively on arm64. The emulator starts these
samples on container images Microsoft publishes for amd64 only, with no
arm64 manifest to pull:

  • mcr.microsoft.com/oryx/ backs every az webapp create --runtime ... code deployment, covering all web-app-* samples and
    eventhubs (it deploys a dashboard web app)
  • mcr.microsoft.com/azure-app-service/java backs servicebus/java
  • mcr.microsoft.com/mssql/server backs web-app-sql-database

The remaining six sample families are arch-agnostic: Function Apps build
their image from a multi-arch base, and the custom-image Web App and ACI
samples run an image the sample builds itself.

run-samples.sh now declares this per sample in ARM64_SAMPLE_DIRS and
emits an arches field from --list; build-matrix.sh expands each
selected test into one job per supported architecture, mapping amd64 to
ubuntu-22.04 and arm64 to ubuntu-22.04-arm. This takes a full run from
34 to 48 jobs. The MSSQL ODBC install is skipped on arm64, where no
sample needs it.

Note that "amd64-only" means not native: the emulator never pins
--platform, so Docker Desktop on Apple Silicon runs these under Rosetta,
just more slowly. run-samples.sh therefore warns rather than skips on an
arm64 host, with SKIP_AMD64_ONLY=1 to opt out. CI schedules only the
natively-supported samples because GitHub's arm64 runners register no
binfmt/QEMU.

The Samples CI only ever ran on amd64, so nothing caught the Azure
Functions arm64 breakage that a reviewer hit on Apple Silicon (fixed in
localstack-pro#8102). Add an architecture axis to the test matrix.

Not every sample can run natively on arm64. The emulator starts these
samples on container images Microsoft publishes for amd64 only, with no
arm64 manifest to pull:

  - mcr.microsoft.com/oryx/<platform> backs every `az webapp create
    --runtime ...` code deployment, covering all web-app-* samples and
    eventhubs (it deploys a dashboard web app)
  - mcr.microsoft.com/azure-app-service/java backs servicebus/java
  - mcr.microsoft.com/mssql/server backs web-app-sql-database

The remaining six sample families are arch-agnostic: Function Apps build
their image from a multi-arch base, and the custom-image Web App and ACI
samples run an image the sample builds itself.

run-samples.sh now declares this per sample in ARM64_SAMPLE_DIRS and
emits an `arches` field from --list; build-matrix.sh expands each
selected test into one job per supported architecture, mapping amd64 to
ubuntu-22.04 and arm64 to ubuntu-22.04-arm. This takes a full run from
34 to 48 jobs. The MSSQL ODBC install is skipped on arm64, where no
sample needs it.

Note that "amd64-only" means not *native*: the emulator never pins
--platform, so Docker Desktop on Apple Silicon runs these under Rosetta,
just more slowly. run-samples.sh therefore warns rather than skips on an
arm64 host, with SKIP_AMD64_ONLY=1 to opt out. CI schedules only the
natively-supported samples because GitHub's arm64 runners register no
binfmt/QEMU.
@DrisDary
DrisDary requested a review from a team as a code owner July 28, 2026 19:21
@DrisDary
DrisDary marked this pull request as draft July 28, 2026 19:21

@paolosalvatori paolosalvatori left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — arm64 sample coverage in CI

Nice piece of work: the arch axis is threaded through cleanly, the amd64-only classification is well reasoned rather than guessed, and the JOB_COUNT-from-the-expanded-matrix detail is exactly the subtlety that usually gets missed. I verified the load-bearing claims against the registries and the emulator source rather than taking them on trust — they all hold (table below). Three inline comments, all hardening; nothing blocking.

Noting this is still a draft, and that bblommers has already approved.

1. Rules applied

  • localstack-pro-azure/CLAUDE.md
  • .claude/rules/azure/common/: coding-style.md, hooks.md, patterns.md, security.md, testing.md
  • .claude/rules/azure/python/: coding-style.md, hooks.md, patterns.md, security.md, testing.md
  • .claude/rules/azure/scripts/: coding-style.md, hooks.md, patterns.md, security.md, testing.md — the ingestion table in my run instructions names a shell/ directory; the real one is scripts/, which is what I read

Rules were re-read against origin/main (3fbd587b97); the delta since my last run is the mypy → ty lint-gate change plus a new emulator/azure-event-hubs.md, none of which applies to a PR with no Python. No expected rule file was missing. They are path-scoped to localstack-pro-azure/**, so here they were applied as conventions (strict mode, quoting, explicit error handling, documenting deliberate gaps), not as hard law. bash -n passes on both scripts; shellcheck is not installed on my machine, so the scripts/hooks.md static-analysis step could not be run.

2. Existing comments

bblommers approved with an empty body and no inline comments, so nothing was already covered; no bot comments.

3. Findings

Three inline comments: one MEDIUM on build-matrix.sh (an unrecognised arch filter produces zero jobs and a green workflow) and two LOW on run-samples.sh (ARM64_SAMPLE_DIRS drift guard; the unknown-architecture fallback). Each suggestion was executed locally before posting.

4. Clean files

  • .github/workflows/run-samples.yml — the new arch input is routed through env instead of interpolated into the shell, matching the existing template-injection defence; ubuntu-22.04-arm is the correct label; the MSSQL ODBC guard is right (that sample is amd64-only); and the wait -t 300 bump is the better fix here — bounding concurrency with max-parallel would not help, since the account's runner-concurrency limit already caps how many of the 48 jobs run at once.
  • README.md — every claim in the new section checks out.

5. Verdict

Approve. No HIGH findings: the architecture split is correct, every factual claim verifies, and both scripts behave as intended when exercised — the MEDIUM is a missing input guard and the two LOWs are drift/documentation hardening.

Verified claims

The service-emulation-parity part of my review process doesn't apply (this is CI plumbing and docs, not an emulated service), so I verified the PR's load-bearing factual claims instead:

Claim Verified
oryx/build, azure-app-service/java, mssql/server:2022-latest are amd64-only ✅ no manifest list; config blobs report linux/amd64
Those are the images the emulator actually starts platform_utilities.py (oryx/build:github-actions-debian-bookworm-20260415.1), java_deployer.py (java:21.0.9-r260110), sql/models.py (mssql/server:2022-latest)
The emulator never pins --platform ✅ the only --platform in the Azure service code is Oryx's own language flag (oryx build --platform python)
Function Apps build from a multi-arch base function_deployer.py builds FROM python:*-slim, node:*-slim, mcr.microsoft.com/dotnet/aspnet — all publish arm64
The arm64 samples' dependencies are multi-arch ✅ Azurite is amd64+arm64; Service Bus and Front Door start no container at all
aci-blob-storage / web-app-custom-image build their own image ✅ plain docker build, no --platform, and no linux-x64/RID pinning anywhere in the six arm64 families
localstack-pro draws the same line for Web Apps test_deploy_zip_without_basic_auth (L231) and test_deploy_zip_that_accesses_storage (L349) carry @core_markers.only_on_amd64; test_deploy_zip/_async do not
"from 34 to 48 jobs" --list emits 34 tests, 14 arm64-capable → 48 expanded jobs; the three filters give 48 / 34 / 14 with the right runner labels
The arm64 runner can run this workflow ubuntu-22.04-arm ships Azure CLI 2.88.0, Node 22, Docker 28, Terraform and the .NET 10 SDK — nothing the workflow depends on is missing
localstack/localstack-azure:latest is pullable on arm64 ✅ manifest lists linux/amd64 + linux/arm64

Two notes rather than findings

  • The localstack-pro#8102 caveat can be reworded. That PR merged 2026-07-28 and localstack/localstack-azure:latest was pushed 2026-07-29 as a multi-arch image, so the dependency is already satisfied on latest. Turning the README note from a prerequisite into a "fixed since" pointer before undrafting would stop it reading as a blocker.
  • Renaming every job is safe. The matrix name now carries an [amd64]/[arm64] suffix, which changes all existing check names; main has no branch protection and no required status checks, so nothing breaks.

Forward-looking: native arm64 coverage of the web-app-* family is blocked upstream by Microsoft's amd64-only Oryx images, not by anything here. If MCR ever publishes arm64 Oryx, ARM64_SAMPLE_DIRS is the single place to extend — which is exactly the right shape for this to have.

Comment on lines +10 to +11
# "both" runs each test on every architecture it supports; "amd64"/"arm64" restrict it to one
ARCH_FILTER="${3:-both}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MEDIUM | An unrecognised arch filter yields zero jobs and a green workflow

Description: An ARCH_FILTER value that is not exactly both/amd64/arm64 matches no arch in the select, so the expansion produces an empty matrix, has_tests=false, and the whole scripts job is skipped while the workflow still reports success. I confirmed it by running the script directly:

$ ./.github/scripts/build-matrix.sh all "" "aarch64"
Run mode: all | Arch filter: aarch64 | Total tests: 34
No tests to run.        # exit 0, has_tests=false, matrix={"include":[]}

The new JOB_COUNT check is the right call for a genuine no-match, but it also makes a typo indistinguishable from the legitimate "nothing changed" outcome, so the failure is silent instead of loud. In CI the choice dropdown constrains the value, so the realistic triggers are a local invocation, a future caller, or a typo in DEFAULT_ARCH.

Rules applied: .claude/rules/azure/common/coding-style.md, "Error Handling — Handle errors explicitly at every level / Never silently swallow exceptions".

Verified this suggestion keeps both/amd64/arm64 at 48/34/14 jobs, still defaults an empty $3 to both, and exits 1 on anything else:

Suggested change
# "both" runs each test on every architecture it supports; "amd64"/"arm64" restrict it to one
ARCH_FILTER="${3:-both}"
# "both" runs each test on every architecture it supports; "amd64"/"arm64" restrict it to one
ARCH_FILTER="${3:-both}"
case "$ARCH_FILTER" in
both | amd64 | arm64) ;;
*) echo "Invalid arch filter: '$ARCH_FILTER' (expected both, amd64 or arm64)" >&2; exit 1 ;;
esac

Comment thread run-samples.sh
"samples/function-app-service-bus/dotnet"
"samples/function-app-storage-http/dotnet"
"samples/web-app-custom-image/python"
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOW | A stale ARM64_SAMPLE_DIRS entry silently drops arm64 coverage

Description: supports_arm64 compares entries by exact string, so renaming a sample directory without updating this second list silently demotes it to amd64-only, and CI stays green. I simulated one typo'd entry (.../dotnet.../csharp) and the run went from 48 to 45 jobs (14 → 11 arm64 tests) with exit code 0 and no diagnostic. Since arch support is now declared away from the sample arrays, a cheap consistency guard keeps the two from drifting.

Rules applied: .claude/rules/azure/common/coding-style.md, "DRY — Avoid copy-paste implementation drift"; .claude/rules/azure/scripts/coding-style.md, "Reuse captured values; don't hardcode or repeat them".

Verified: passes unchanged on the current list, and fails with the message below when an entry no longer matches a registered sample.

Suggested change
)
)
# Fail loudly if an entry no longer matches a registered sample: a rename would otherwise
# silently drop that sample's arm64 coverage while CI stays green.
for _arm64_dir in "${ARM64_SAMPLE_DIRS[@]}"; do
if ! printf '%s\n' "${ALL_SAMPLES[@]}" | cut -d'|' -f1 | sed -E 's#/(terraform|bicep)$##' | grep -qxF "$_arm64_dir"; then
echo >&2 "ARM64_SAMPLE_DIRS entry '$_arm64_dir' matches no registered sample - fix the path or remove the entry."
exit 1
fi
done

Comment thread run-samples.sh Outdated
Comment on lines +129 to +135
# Normalise `uname -m` to the architecture names used above
host_arch() {
case "$(uname -m)" in
aarch64 | arm64) echo "arm64" ;;
*) echo "amd64" ;;
esac
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOW | Unknown architectures are silently reported as amd64

Description: Every uname -m that is not aarch64/arm64 is reported as amd64, so on an exotic host (armv7l, ppc64le, riscv64) the new arm64 guidance never fires and an amd64-only sample fails deep inside a deployment instead of warning first. The fallback itself is a reasonable choice — the point is that the ingested rule asks an approximation to say so and to state which way it errs.

Rules applied: .claude/rules/azure/scripts/coding-style.md, "Document deliberate gaps — If a script intentionally skips or approximates a step, say so in a comment and state the consequence".

Verified the note goes to stderr, so HOST_ARCH=$(host_arch) still captures just the value (aarch64arm64, x86_64amd64, ppc64le/armv7lamd64 + note):

Suggested change
# Normalise `uname -m` to the architecture names used above
host_arch() {
case "$(uname -m)" in
aarch64 | arm64) echo "arm64" ;;
*) echo "amd64" ;;
esac
}
# Normalise `uname -m` to the architecture names used above. Anything not recognisably
# arm64 is reported as amd64, so an exotic host (armv7l, ppc64le, ...) takes the amd64
# path and gets no arm64 warning: it errs toward "run it and let the deployment fail"
# rather than skipping silently.
host_arch() {
case "$(uname -m)" in
aarch64 | arm64) echo "arm64" ;;
x86_64 | amd64) echo "amd64" ;;
*)
echo >&2 "Note: unrecognised architecture '$(uname -m)'; treating it as amd64."
echo "amd64"
;;
esac
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants