Initial support of ARM architecture - #321
Merged
Merged
Conversation
Contributor
WalkthroughRuntime CPU architecture detection was added and propagated: CI workflow, Ansible provisioning, and Python utilities now detect and map host architecture instead of assuming amd64; artifact URLs/names and container tag queries were adjusted to use the mapped architecture and version-aware naming. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ansible/roles/debian_base/tasks/60-go-task.yaml (1)
24-26: 💤 Low valueVersion default mismatch with group_vars.
The inline default
v3.50.0differs from the group_vars definition (v3.44.1peransible/inventory/group_vars/debian/vars.yaml). While the group_vars value takes precedence when loaded, this hidden fallback could cause unexpected version drift if the inventory variable is ever removed.Consider aligning the fallback with group_vars or removing the default to make the dependency explicit:
♻️ Suggested fix
- name: Set go-task version ansible.builtin.set_fact: - debian_base_go_task_version: "{{ debian_base_go_task_version | default('v3.50.0') }}" + debian_base_go_task_version: "{{ debian_base_go_task_version | default('v3.44.1') }}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible/roles/debian_base/tasks/60-go-task.yaml` around lines 24 - 26, The inline default for debian_base_go_task_version in the "Set go-task version" task mismatches the group_vars value; update the task so the fallback aligns with group_vars or remove the inline default to force using the inventory value: either change the default string from 'v3.50.0' to 'v3.44.1' (matching group_vars) or remove the default expression (debian_base_go_task_version | default(...)) so the playbook fails/relies on the inventory variable instead.scripts/infra-mcp/utils/models.py (1)
9-14: ⚡ Quick winCentralize architecture mapping/default logic to one shared utility.
_arch_map+ default architecture computation are now duplicated acrossscripts/infra-mcp/utils/models.py,scripts/infra-mcp/utils/constants.py, andscripts/infra-mcp/tools/get_container_tags.py. Consolidating this avoids drift and inconsistent platform behavior.Also applies to: 31-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/utils/models.py` around lines 9 - 14, The _arch_map dict and _default_architecture() logic are duplicated; extract them into a single shared utility (e.g., create a module-level function like get_default_architecture() and exported ARCH_MAP constant) and replace local copies—remove _arch_map and _default_architecture from models.py and from other files and import the shared get_default_architecture/ARCH_MAP instead; ensure callers that used _default_architecture() now call get_default_architecture(), and update any tests or references to the old names to use the new centralized symbols..github/workflows/pre-commit.yml (1)
34-35: ⚡ Quick winFail fast on unsupported architectures in the terraform-docs installer.
The current
sedmapping silently passes unknownuname -mvalues into the URL, which turns into a harder-to-diagnose download/extract failure. Add explicit mapping + early exit.Proposed patch
- ARCH=$(uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/') + RAW_ARCH="$(uname -m)" + case "${RAW_ARCH}" in + x86_64) ARCH="amd64" ;; + aarch64|arm64) ARCH="arm64" ;; + *) echo "Unsupported architecture: ${RAW_ARCH}" >&2; exit 1 ;; + esac curl -sSLo ./terraform-docs.tar.gz "https://terraform-docs.io/dl/v${VERSION}/terraform-docs-v${VERSION}-$(uname)-${ARCH}.tar.gz"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pre-commit.yml around lines 34 - 35, The ARCH assignment currently uses sed to map uname -m but allows unknown values to pass through; update the logic around ARCH and the subsequent curl invocation so unsupported architectures cause an immediate non-zero exit. Concretely, replace the fragile sed mapping for ARCH (the line setting ARCH=$(uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/') ) with an explicit mapping/check that sets ARCH only for supported values (e.g., map x86_64→amd64 and aarch64→arm64) and otherwise prints a clear error and exits; then only run the curl download line (the curl -sSLo ./terraform-docs.tar.gz "...${ARCH}...") when ARCH is valid. Ensure the failure path uses a non-zero exit code and an explanatory message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/infra-mcp/tools/get_container_tags.py`:
- Around line 49-52: The _parse_arch function currently splits arch into parts
and sets os_part and arch_part without checking for empty strings, so inputs
like "linux/" yield an empty arch_part; update _parse_arch to treat empty
components as missing by using parts[0] if parts and parts[0] else "linux" for
os_part and for arch_part use parts[1] if len(parts) > 1 and parts[1] else
_arch_map.get(platform.machine(), platform.machine()); ensure references to
parts, os_part, arch_part, _arch_map, and platform.machine() are used so the
default host architecture is applied when the second component is empty.
---
Nitpick comments:
In @.github/workflows/pre-commit.yml:
- Around line 34-35: The ARCH assignment currently uses sed to map uname -m but
allows unknown values to pass through; update the logic around ARCH and the
subsequent curl invocation so unsupported architectures cause an immediate
non-zero exit. Concretely, replace the fragile sed mapping for ARCH (the line
setting ARCH=$(uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/') ) with an
explicit mapping/check that sets ARCH only for supported values (e.g., map
x86_64→amd64 and aarch64→arm64) and otherwise prints a clear error and exits;
then only run the curl download line (the curl -sSLo ./terraform-docs.tar.gz
"...${ARCH}...") when ARCH is valid. Ensure the failure path uses a non-zero
exit code and an explanatory message.
In `@ansible/roles/debian_base/tasks/60-go-task.yaml`:
- Around line 24-26: The inline default for debian_base_go_task_version in the
"Set go-task version" task mismatches the group_vars value; update the task so
the fallback aligns with group_vars or remove the inline default to force using
the inventory value: either change the default string from 'v3.50.0' to
'v3.44.1' (matching group_vars) or remove the default expression
(debian_base_go_task_version | default(...)) so the playbook fails/relies on the
inventory variable instead.
In `@scripts/infra-mcp/utils/models.py`:
- Around line 9-14: The _arch_map dict and _default_architecture() logic are
duplicated; extract them into a single shared utility (e.g., create a
module-level function like get_default_architecture() and exported ARCH_MAP
constant) and replace local copies—remove _arch_map and _default_architecture
from models.py and from other files and import the shared
get_default_architecture/ARCH_MAP instead; ensure callers that used
_default_architecture() now call get_default_architecture(), and update any
tests or references to the old names to use the new centralized symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4bb80773-ce81-46a0-8ba9-6e7918235166
📒 Files selected for processing (5)
.github/workflows/pre-commit.ymlansible/roles/debian_base/tasks/60-go-task.yamlscripts/infra-mcp/tools/get_container_tags.pyscripts/infra-mcp/utils/constants.pyscripts/infra-mcp/utils/models.py
bubacoder
force-pushed
the
feature/arm-support
branch
from
May 1, 2026 18:05
bf295da to
890c838
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pre-commit.yml (1)
35-39:⚠️ Potential issue | 🟠 MajorAdd checksum validation when installing terraform-docs.
Lines 35–39 download and install an executable without verifying its integrity. While
terraform-docs-v0.20.0.sha256sumis available in the GitHub releases, the suggested fix references an incorrect URL and contains an undefined variable.Correct approach: download the checksum file from GitHub releases (not terraform-docs.io), then validate the binary before extraction. The binary filename format should use
$(uname)which returnsLinux(matching the checksum entries), not an undefined${OS}variable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pre-commit.yml around lines 35 - 39, The workflow currently downloads and installs terraform-docs without verifying integrity; update the install block that handles terraform-docs, terraform-docs.tar.gz, VERSION and ARCH so it first downloads the release checksum from the GitHub releases (e.g., terraform-docs-v${VERSION}.sha256sum from the repo's releases), use $(uname) (not ${OS}) to match the checksum entry for the platform, validate the downloaded terraform-docs tarball with sha256sum (e.g., extract the matching line or use grep to feed sha256sum -c) and only proceed to extract, chmod +x, move /usr/local/bin/terraform-docs and remove the tarball if the checksum verification succeeds; fail the job if verification fails.
🧹 Nitpick comments (2)
scripts/infra-mcp/utils/models.py (1)
9-14: ⚡ Quick winCentralize architecture mapping in one module to prevent drift
The
_arch_map+ default-resolution logic is duplicated withscripts/infra-mcp/utils/constants.py. Please source this from a single helper/constant so future architecture additions only change in one place.Proposed refactor
-import platform from dataclasses import dataclass, field +from .constants import DEFAULT_CONTAINER_ARCHITECTURE -_arch_map = {"x86_64": "amd64", "aarch64": "arm64", "armv7l": "arm"} - - def _default_architecture() -> str: - arch = _arch_map.get(platform.machine(), platform.machine()) - return f"linux/{arch}" + return DEFAULT_CONTAINER_ARCHITECTURE🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/utils/models.py` around lines 9 - 14, The _arch_map and _default_architecture logic is duplicated; remove the copy in models.py and import the centralized mapping/function from the existing constants module instead: delete _arch_map and _default_architecture from scripts/infra-mcp/utils/models.py and replace their usage with an import from constants (e.g., from utils.constants import _arch_map or preferably a public helper like default_architecture), update any references in models.py to call the imported function or use the shared mapping, and run tests/lint to ensure no unresolved names remain..github/workflows/pre-commit.yml (1)
34-35: ⚡ Quick winAdd explicit architecture validation and fail-fast download behavior.
Line 34 currently falls through for unknown
uname -mvalues, and Line 35 usescurlwithout-f, which can mask a bad URL untiltarfails. A small guard makes failures immediate and clearer.Proposed hardening
- ARCH=$(uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/') - curl -sSLo ./terraform-docs.tar.gz "https://terraform-docs.io/dl/v${VERSION}/terraform-docs-v${VERSION}-$(uname)-${ARCH}.tar.gz" + case "$(uname -m)" in + x86_64|amd64) ARCH="amd64" ;; + aarch64|arm64) ARCH="arm64" ;; + *) echo "Unsupported architecture: $(uname -m)" >&2; exit 1 ;; + esac + OS="$(uname -s | tr '[:upper:]' '[:lower:]')" + curl -fsSLo ./terraform-docs.tar.gz "https://terraform-docs.io/dl/v${VERSION}/terraform-docs-v${VERSION}-${OS}-${ARCH}.tar.gz"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pre-commit.yml around lines 34 - 35, The ARCH assignment and curl download need hardening: validate the result of ARCH after the sed mapping (the variable set from `uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/'`) and if it is empty or not one of the supported values, emit a clear error and exit immediately; also update the `curl` invocation that builds the terraform-docs URL (the command assigning/using `curl -sSLo ./terraform-docs.tar.gz "https://.../terraform-docs-v${VERSION}-$(uname)-${ARCH}.tar.gz"`) to include fail-fast behavior (e.g., add `-f`) so a non-2xx response causes curl to fail immediately and surface a clear error before `tar` runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/pre-commit.yml:
- Around line 35-39: The workflow currently downloads and installs
terraform-docs without verifying integrity; update the install block that
handles terraform-docs, terraform-docs.tar.gz, VERSION and ARCH so it first
downloads the release checksum from the GitHub releases (e.g.,
terraform-docs-v${VERSION}.sha256sum from the repo's releases), use $(uname)
(not ${OS}) to match the checksum entry for the platform, validate the
downloaded terraform-docs tarball with sha256sum (e.g., extract the matching
line or use grep to feed sha256sum -c) and only proceed to extract, chmod +x,
move /usr/local/bin/terraform-docs and remove the tarball if the checksum
verification succeeds; fail the job if verification fails.
---
Nitpick comments:
In @.github/workflows/pre-commit.yml:
- Around line 34-35: The ARCH assignment and curl download need hardening:
validate the result of ARCH after the sed mapping (the variable set from `uname
-m | sed 's/x86_64/amd64/;s/aarch64/arm64/'`) and if it is empty or not one of
the supported values, emit a clear error and exit immediately; also update the
`curl` invocation that builds the terraform-docs URL (the command
assigning/using `curl -sSLo ./terraform-docs.tar.gz
"https://.../terraform-docs-v${VERSION}-$(uname)-${ARCH}.tar.gz"`) to include
fail-fast behavior (e.g., add `-f`) so a non-2xx response causes curl to fail
immediately and surface a clear error before `tar` runs.
In `@scripts/infra-mcp/utils/models.py`:
- Around line 9-14: The _arch_map and _default_architecture logic is duplicated;
remove the copy in models.py and import the centralized mapping/function from
the existing constants module instead: delete _arch_map and
_default_architecture from scripts/infra-mcp/utils/models.py and replace their
usage with an import from constants (e.g., from utils.constants import _arch_map
or preferably a public helper like default_architecture), update any references
in models.py to call the imported function or use the shared mapping, and run
tests/lint to ensure no unresolved names remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 99820b4c-0340-45b2-8f1c-a4804aaa188d
📒 Files selected for processing (6)
.github/workflows/pre-commit.ymlansible/inventory/group_vars/debian/vars.yamlansible/roles/debian_base/tasks/60-go-task.yamlscripts/infra-mcp/tools/get_container_tags.pyscripts/infra-mcp/utils/constants.pyscripts/infra-mcp/utils/models.py
✅ Files skipped from review due to trivial changes (3)
- ansible/inventory/group_vars/debian/vars.yaml
- scripts/infra-mcp/utils/constants.py
- scripts/infra-mcp/tools/get_container_tags.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ansible/roles/debian_base/tasks/60-go-task.yaml
bubacoder
force-pushed
the
feature/arm-support
branch
from
May 1, 2026 19:08
890c838 to
0eca48a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/infra-mcp/utils/models.py (1)
9-14: ⚡ Quick winDeduplicate architecture default logic with
utils/constants.py.Lines 9-14 and Line 31 re-implement logic already present in
scripts/infra-mcp/utils/constants.py(DEFAULT_CONTAINER_ARCHITECTURE), which can drift over time. Centralizing this default in one place will reduce maintenance risk.♻️ Proposed simplification
-import platform -from dataclasses import dataclass, field +from dataclasses import dataclass +from .constants import DEFAULT_CONTAINER_ARCHITECTURE -_arch_map = {"x86_64": "amd64", "aarch64": "arm64", "armv7l": "arm"} - - -def _default_architecture() -> str: - arch = _arch_map.get(platform.machine(), platform.machine()) - return f"linux/{arch}" - `@dataclass` class ContainerTagFinderArgs: @@ - architecture: str = field(default_factory=_default_architecture) + architecture: str = DEFAULT_CONTAINER_ARCHITECTUREAlso applies to: 31-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/infra-mcp/utils/models.py` around lines 9 - 14, The architecture default logic in _arch_map and _default_architecture duplicates DEFAULT_CONTAINER_ARCHITECTURE; remove the local mapping and have _default_architecture (or callers) use the shared DEFAULT_CONTAINER_ARCHITECTURE from constants instead. Import DEFAULT_CONTAINER_ARCHITECTURE, replace the body of _default_architecture to return that constant (or eliminate the helper and use the constant at call sites), and remove the now-redundant _arch_map; update any other local uses (the duplicate at line 31) to reference DEFAULT_CONTAINER_ARCHITECTURE so the single canonical value is used project-wide.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible/bootstrap-ansible.sh`:
- Around line 20-21: The UBUNTU_CODENAME assignment uses VERSION_CODENAME
without a default so the script will fail under set -u if that key is missing;
modify the command-substitution that sets UBUNTU_CODENAME so it safely handles
an unset VERSION_CODENAME (e.g., use parameter expansion to supply an empty or
safe default or fall back to another retrieval method) so the script does not
exit under strict mode; update the expression that sets UBUNTU_CODENAME and keep
references to VERSION_CODENAME and the current command-substitution form so
reviewers can find and verify the change.
In `@ansible/roles/debian_base/tasks/60-go-task.yaml`:
- Around line 28-31: The get_url task "Download go-task .deb package" currently
downloads the artifact without verification; add a checksum parameter to the
task: checksum: "sha256:{{ debian_base_go_task_sha256 }}" and ensure the
variable debian_base_go_task_sha256 is defined (one value per architecture)
alongside debian_base_go_task_version/debian_base_task_arch in your group vars;
verify the checksum string matches the upstream task_checksums.txt for the
version used so get_url will validate the downloaded .deb before passing it to
apt.
---
Nitpick comments:
In `@scripts/infra-mcp/utils/models.py`:
- Around line 9-14: The architecture default logic in _arch_map and
_default_architecture duplicates DEFAULT_CONTAINER_ARCHITECTURE; remove the
local mapping and have _default_architecture (or callers) use the shared
DEFAULT_CONTAINER_ARCHITECTURE from constants instead. Import
DEFAULT_CONTAINER_ARCHITECTURE, replace the body of _default_architecture to
return that constant (or eliminate the helper and use the constant at call
sites), and remove the now-redundant _arch_map; update any other local uses (the
duplicate at line 31) to reference DEFAULT_CONTAINER_ARCHITECTURE so the single
canonical value is used project-wide.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 81754a5d-b518-492c-a2c3-a58242d150e9
📒 Files selected for processing (7)
.github/workflows/pre-commit.ymlansible/bootstrap-ansible.shansible/inventory/group_vars/debian/vars.yamlansible/roles/debian_base/tasks/60-go-task.yamlscripts/infra-mcp/tools/get_container_tags.pyscripts/infra-mcp/utils/constants.pyscripts/infra-mcp/utils/models.py
✅ Files skipped from review due to trivial changes (3)
- ansible/inventory/group_vars/debian/vars.yaml
- scripts/infra-mcp/utils/constants.py
- scripts/infra-mcp/tools/get_container_tags.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pre-commit.yml
Comment on lines
+20
to
+21
| UBUNTU_CODENAME=$(. /etc/os-release && echo "$VERSION_CODENAME") | ||
|
|
Contributor
There was a problem hiding this comment.
Harden codename extraction for strict mode (set -u).
At Line 20, $VERSION_CODENAME is expanded without a default. If that key is absent in /etc/os-release, the script exits immediately under set -u.
Suggested fix
- UBUNTU_CODENAME=$(. /etc/os-release && echo "$VERSION_CODENAME")
+ . /etc/os-release
+ UBUNTU_CODENAME="${VERSION_CODENAME:-${UBUNTU_CODENAME:-}}"
+ if [ -z "$UBUNTU_CODENAME" ]; then
+ echo "Unable to determine Ubuntu codename from /etc/os-release"
+ exit 1
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| UBUNTU_CODENAME=$(. /etc/os-release && echo "$VERSION_CODENAME") | |
| . /etc/os-release | |
| UBUNTU_CODENAME="${VERSION_CODENAME:-${UBUNTU_CODENAME:-}}" | |
| if [ -z "$UBUNTU_CODENAME" ]; then | |
| echo "Unable to determine Ubuntu codename from /etc/os-release" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ansible/bootstrap-ansible.sh` around lines 20 - 21, The UBUNTU_CODENAME
assignment uses VERSION_CODENAME without a default so the script will fail under
set -u if that key is missing; modify the command-substitution that sets
UBUNTU_CODENAME so it safely handles an unset VERSION_CODENAME (e.g., use
parameter expansion to supply an empty or safe default or fall back to another
retrieval method) so the script does not exit under strict mode; update the
expression that sets UBUNTU_CODENAME and keep references to VERSION_CODENAME and
the current command-substitution form so reviewers can find and verify the
change.
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
1 participant
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.
Summary by CodeRabbit
Chores
Bug Fixes