-
Notifications
You must be signed in to change notification settings - Fork 7
LCORE-858: konflux - activate ARM build #78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Tekton CI/CD configuration '.tekton/dataverse-exporter-pull-request.yaml', '.tekton/dataverse-exporter-push.yaml', '.tekton/lightspeed-to-dataverse-exporter-pull-request.yaml', '.tekton/lightspeed-to-dataverse-exporter-push.yaml' |
Add linux-mxlarge/arm64 (and linux/x86_64) to build-platforms; change prefetch-input to reference requirements.x86_64.txt and requirements.aarch64.txt instead of requirements.txt. |
Container build configuration Containerfile |
COPY now includes requirements.*.txt; pip install uses -r requirements.$(uname -m).txt to select architecture-specific requirements. |
Dependency manifests requirements.x86_64.txt, requirements.aarch64.txt, requirements.txt |
Add autogenerated, pinned per-architecture manifests requirements.x86_64.txt and requirements.aarch64.txt; remove the previous requirements.txt. |
Build tooling & docs Makefile, README.md |
Add requirements Make target that generates per-arch requirements via uv pip compile; update README with hermetic build / per-arch requirements documentation. |
Possibly related PRs
- LCORE-858 konflux: hermetic build #77 — Alters Tekton prefetch inputs and build-platform settings, overlapping with multi-arch prefetch and build configuration changes.
Suggested reviewers
- matysek
Pre-merge checks and finishing touches
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The PR title clearly and specifically describes the main change: enabling ARM builds in Konflux by adding linux-mxlarge/arm64 platform support across all build configuration files. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✨ Finishing touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Tip
📝 Customizable high-level summaries are now available in beta!
You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
- Provide your own instructions using the
high_level_summary_instructionssetting. - Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
- Use
high_level_summary_in_walkthroughto move the summary from the description to the walkthrough section.
Example instruction:
"Divide the high-level summary into five sections:
- 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
- 📓 References — List relevant issues, discussions, documentation, or related PRs.
- 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
- 📊 Contributor Summary — Include a Markdown table showing contributions:
| Contributor | Lines Added | Lines Removed | Files Changed |- ✔️ Additional Notes — Add any extra reviewer context.
Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.
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.
Comment @coderabbitai help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
requirements.x86_64.txt (1)
1-205: Autogenerated lockfile looks consistent; keep it tool-managedThe uv header plus fully hashed pins look good. Just keep treating this as generated output (no manual edits) and re-run
uv pip compilewhenpyproject.tomlchanges so x86_64 stays in sync with the aarch64 lockfile.requirements.aarch64.txt (1)
1-431: Cross-arch dependency versions are skewed; decide if that’s intentionalThe aarch64 lockfile is well-formed, but it doesn’t match x86_64 for several key packages (examples:
kubernetes34.1.0 vs 33.1.0,requests2.32.5 vs 2.32.4,pydantic2.12.5 vs 2.11.7,rich14.2.0 vs 14.1.0,urllib32.3.0 vs 2.5.0).If you want identical behavior across architectures, consider re-running
uv pip compilefor both platforms in a single sweep so they converge on the same versions where possible; otherwise, it’s worth explicitly acknowledging that aarch64 may be running slightly different dependency stacks than x86_64.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.tekton/dataverse-exporter-pull-request.yaml(1 hunks).tekton/dataverse-exporter-push.yaml(1 hunks)Containerfile(1 hunks)requirements.aarch64.txt(1 hunks)requirements.txt(0 hunks)requirements.x86_64.txt(1 hunks)
💤 Files with no reviewable changes (1)
- requirements.txt
🧰 Additional context used
🪛 GitHub Actions: Build and validate image for PR
Containerfile
[error] 1-1: COPY pyproject.toml LICENSE README.md requirements.txt ./ failed: stat: '/requirements.txt' not found in build context. buildah exited with code 125.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Konflux kflux-prd-rh02 / dataverse-exporter-on-pull-request
🔇 Additional comments (3)
.tekton/dataverse-exporter-pull-request.yaml (1)
30-38: Build platforms and prefetch-input wiring for ARM look consistentAdding
linux-mxlarge/arm64tobuild-platformsand pointingprefetch-inputat bothrequirements.x86_64.txtandrequirements.aarch64.txtmatches the new per-arch dependency layout. Just make sure your Konflux/multi-platform-controller setup actually has alinux-mxlarge/arm64builder available so these matrix builds don’t get stuck waiting for capacity..tekton/dataverse-exporter-push.yaml (1)
27-35: Push pipeline ARM platform and prefetch configuration match PR pipelineThe push PipelineRun now mirrors the PR pipeline:
linux-mxlarge/arm64inbuild-platformsand prefetch configured with both per-arch requirements files. That keeps CI behavior consistent across PR and push.Just as for the PR pipeline, please confirm that
linux-mxlarge/arm64is a valid platform in your Konflux setup so multi-arch builds actually run.Containerfile (1)
20-27: I apologize, but I'm unable to proceed with verification due to persistent repository access issues. The repositorylightspeed-core/lightspeed-to-dataverse-exportercannot be cloned, and it's not publicly indexed, which prevents me from:
- Examining the actual Containerfile content
- Confirming the presence or absence of
requirements.txtand per-arch requirements files- Verifying the build failure mentioned in the review
- Validating whether the suggested fix is appropriate
To complete this verification, I would need one of the following:
- Repository access: Permission to clone the private repository
- Direct context: The pull request URL, commit hash, or build logs showing the actual failure
- File contents: You could paste the current Containerfile and list of files in the build context
Without this information, I cannot definitively verify whether the review comment is accurate or needs adjustment.
Signed-off-by: Haoyu Sun <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
1-1: Addrequirementstarget to.PHONYdeclaration.The new
requirementstarget (lines 53–55) is not listed in the.PHONYdeclaration. This can cause issues if a file namedrequirementsexists in the repo.-.PHONY: help install install-dev format lint test test-cov check build run-container build-and-push deploy clean-deployment-stage +.PHONY: help install install-dev format lint test test-cov check build run-container build-and-push deploy clean-deployment-stage requirements
🧹 Nitpick comments (1)
Makefile (1)
53-55: Clarify when therequirementstarget is invoked and document its integration.This target generates architecture-specific pinned dependencies, which are referenced by the Tekton pipeline. Clarify:
- Whether these files should be pre-generated and committed to the repo, or generated on-demand during the build
- Whether the pipeline or pre-commit hooks should call this target
- Whether developers need to run
make requirementsmanually after updatingpyproject.tomlConsider adding inline documentation in the Makefile or a note in CONTRIBUTING.md.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.tekton/dataverse-exporter-pull-request.yaml(1 hunks).tekton/dataverse-exporter-push.yaml(1 hunks).tekton/lightspeed-to-dataverse-exporter-pull-request.yaml(1 hunks).tekton/lightspeed-to-dataverse-exporter-push.yaml(1 hunks)Containerfile(1 hunks)Makefile(1 hunks)README.md(1 hunks)requirements.aarch64.txt(1 hunks)requirements.txt(0 hunks)requirements.x86_64.txt(1 hunks)
💤 Files with no reviewable changes (1)
- requirements.txt
✅ Files skipped from review due to trivial changes (2)
- requirements.aarch64.txt
- README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- .tekton/dataverse-exporter-pull-request.yaml
- .tekton/lightspeed-to-dataverse-exporter-push.yaml
- Containerfile
- requirements.x86_64.txt
- .tekton/dataverse-exporter-push.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Red Hat Konflux / lightspeed-to-dataverse-exporter-on-pull-request
- GitHub Check: Konflux kflux-prd-rh02 / dataverse-exporter-on-pull-request
🔇 Additional comments (3)
.tekton/lightspeed-to-dataverse-exporter-pull-request.yaml (2)
32-39: Clarify the build flow and timing of architecture-specific requirements files.The pipeline references
requirements.x86_64.txtandrequirements.aarch64.txt, which are generated by the newrequirementsMakefile target. Ensure these files are generated before the pipeline runs (e.g., committed to the repo, generated in pipeline setup, or pre-generated locally). If they're generated during the pipeline, this dependency must be explicit in the task DAG.Do the generated
requirements*.txtfiles need to be committed to the repository, or are they generated as part of the build process (e.g., in a pipeline init task)?
33-33: I'll help you verify this review comment about the Tekton YAML file. Let me start by examining the file and searching for the relevant task definition.
<function_calls>
Tekton prefetch-dependencies task documentation requirements_files parameter
</function_calls>Now let me examine the actual Tekton YAML file and search for the task definition:
<function_calls>
#!/bin/bashFirst, find and examine the Tekton file
fd -t f "lightspeed-to-dataverse-exporter-pull-request.yaml" 2>/dev/null | head -5
</function_calls>Makefile (1)
53-55: Verification confirms the code is correct —uvsupports both flags and the platform identifiers.
uv pip compileexplicitly supports--python-platformwith Rust-style target triples (includingx86_64-unknown-linux-gnuandaarch64-unknown-linux-gnu), and--generate-hashesworks as expected for hermetic builds. Unlike standard pip-compile which expects PEP-425 platform tags,uvabstracts away this detail and accepts target triple notation directly.
| - name: build-platforms | ||
| value: | ||
| - linux/x86_64 | ||
| - linux-mxlarge/arm64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🌐 Web query:
Konflux build platforms ARM64 tekton pipeline valid identifiers
💡 Result:
Use the platform strings that Konflux’s Multi Platform Controller expects (the PLATFORM Task parameter). Common valid identifiers are the OS/ARCH form, for example:
- linux/arm64
- linux/amd64
- linux/ppc64le
- linux/s390x
- darwin/arm64 (macOS Apple Silicon)
Examples from the Konflux docs show PLATFORM values like linux/arm64 and host entries using the same form; Konflux also appears in pipelines adding arm64 builds. [1][2]
References
- Konflux Multi Platform Controller docs (platform examples and ConfigMap entries). [1]
- OpenShift release notes mentioning Konflux arm64 builds in pipelines. [2]
🏁 Script executed:
# Search for other build-platforms configurations in the repository
fd -e yaml -e yml | xargs grep -l "build-platforms" | head -20Repository: lightspeed-core/lightspeed-to-dataverse-exporter
Length of output: 240
🌐 Web query:
Konflux linux/x86_64 vs linux/amd64 platform identifier accepted formats
💡 Result:
Konflux expects OCI/Go-style platform identifiers (GOOS/GOARCH) — e.g. linux/amd64, linux/arm64, linux/ppc64le — in its platform/config fields. You will still see x86_64 in RPM/repo contexts (package metadata, lockfiles), but Konflux config examples and the multi-platform controller use linux/amd64 (not linux/x86_64). [1][2][3]
Sources:
- Konflux multi-platform controller example (uses linux/amd64, linux/ppc64le). [1]
- Konflux docs (architecture / add-ons) describing supported platforms (amd64, arm64). [2]
- Konflux RPM lockfile docs (uses x86_64 in RPM paths/metadata). [3]
Correct the build-platforms identifiers to match Konflux's OCI/Go-style format.
The declared platforms do not conform to Konflux's platform identifier standard (GOOS/GOARCH):
linux/x86_64should belinux/amd64linux-mxlarge/arm64should belinux/arm64
Konflux expects identifiers like linux/amd64, linux/arm64, linux/ppc64le, not x86_64 or descriptors like mxlarge.
- name: build-platforms
value:
- linux/amd64
- linux/arm64
🤖 Prompt for AI Agents
In .tekton/lightspeed-to-dataverse-exporter-pull-request.yaml around lines 36 to
39, the build-platforms values use non-standard identifiers (linux/x86_64 and
linux-mxlarge/arm64); replace them with Konflux/Go-style GOOS/GOARCH identifiers
by changing linux/x86_64 to linux/amd64 and linux-mxlarge/arm64 to linux/arm64
so the pipeline uses valid platform names.
|
This PR is ready to review, completing the hermetic build on ARM platform, too. |
Summary by CodeRabbit
New Features
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.