chore: switch to container build #2118
Conversation
deboer-tim
force-pushed
the
container-build
branch
from
e980ddc to
ea78667
Compare
deboer-tim
requested review from
benoitf,
bmahabirbu,
jeffmaury and
simonrey1
and removed request for
a team
📝 WalkthroughWalkthroughCI workflows were restructured to a containerized build/publish pipeline: added gated Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Runner as Job runner
participant BuildSvc as buildah/podman
participant Registry as ghcr.io
GH->>Runner: workflow trigger (push / tag)
Runner->>Runner: checkout (adjusted refs/fetch-depth)
Runner->>Runner: run check-builder-changes -> set output builder_required
alt builder_required == 'true'
Runner->>BuildSvc: builder-image job -> build builder image (Containerfile.builder)
BuildSvc-->>Runner: builder image artifact
Runner->>Registry: login & push builder image
Runner->>Registry: create attestation for builder image
end
Runner->>BuildSvc: extension-image job -> build extension image (build/Containerfile)
BuildSvc-->>Runner: extension image artifact
Runner->>Registry: login & push extension image
Runner->>Registry: create attestation for extension image
alt release tag present
Runner->>GH: release job -> publish release (depends on images)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks✅ Passed checks (3 passed)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
.github/workflows/release.yaml (1)
34-105: Critical: Job name mismatch will cause workflow failure.The job is named
build(line 34), but subsequent jobs reference it astag(lines 107, 152, 194). Additionally, the job doesn't declare any outputs, but other jobs referenceneeds.tag.outputs.githubTagandneeds.tag.outputs.bootcExtensionVersion.Apply this diff to fix the job name and add outputs:
jobs: - build: + tag: runs-on: ubuntu-22.04 + outputs: + githubTag: ${{ steps.TAG_UTIL.outputs.githubTag }} + bootcExtensionVersion: ${{ steps.TAG_UTIL.outputs.bootcExtensionVersion }} + releaseId: ${{ steps.create_release.outputs.id }} steps:
🧹 Nitpick comments (5)
build/Containerfile (2)
40-48: Consider consolidating file copying to reduce redundancy.The
package.jsonfile is copied twice (lines 42 and 45), andicon.pngis included in both the*.pngcopy (line 43) and explicitly copied again (line 48). While functionally correct, consolidating these copies would improve clarity.
52-62: Temporary workaround noted for manual node_modules copying.The manual copying of
macadam.js,ssh2, and dependencies is flagged as temporary until the vite script can handle this. This approach works but requires manual updates if dependencies change.Would you like me to open an issue to track the permanent fix via the vite script?
build/Containerfile.builder (2)
24-25: Remove redundantmkdirafterWORKDIR.The
WORKDIRdirective automatically creates the directory if it doesn't exist, making themkdir -pon line 25 redundant.Apply this diff:
WORKDIR /opt/app-root/extension-source -RUN mkdir -p /opt/app-root/extension-source COPY package.json /opt/app-root/extension-source/
29-30: Consider pinning pnpm to a specific minor version for improved build reproducibility.Currently pinned to
pnpm@10, which allows automatic minor and patch updates. To ensure reproducible builds, pin to a specific minor version likepnpm@10.22.0(the latest stable v10 release)..github/workflows/release.yaml (1)
148-192: Extension image workflow looks correct.The extension image job properly builds, publishes, and attests the extension image. Note the use of
archs:(line 165) vsplatforms:in the builder-image job (line 120) - both work with buildah-build, but consistency would be cleaner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/build-next.yaml(2 hunks).github/workflows/release.yaml(2 hunks)RELEASE.md(1 hunks)build/Containerfile(1 hunks)build/Containerfile.builder(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/release.yaml
106-106: job "builder-image" needs job "tag" which does not exist in this workflow
(job-needs)
112-112: property "tag" is not defined in object type {}
(expression)
119-119: property "tag" is not defined in object type {}
(expression)
148-148: job "extension-image" needs job "tag" which does not exist in this workflow
(job-needs)
164-164: property "tag" is not defined in object type {builder-image: {outputs: {}; result: string}}
(expression)
193-193: job "release" needs job "tag" which does not exist in this workflow
(job-needs)
199-199: property "tag" is not defined in object type {builder-image: {outputs: {}; result: string}; extension-image: {outputs: {}; result: string}}
(expression)
206-206: property "tag" is not defined in object type {builder-image: {outputs: {}; result: string}; extension-image: {outputs: {}; result: string}}
(expression)
🪛 markdownlint-cli2 (0.18.1)
RELEASE.md
22-22: Bare URL used
(MD034, no-bare-urls)
🪛 RuboCop (1.81.7)
build/Containerfile.builder
[fatal] 18-18: unexpected token tCOLON
(Using Ruby 2.7 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
🔇 Additional comments (6)
RELEASE.md (1)
22-22: LGTM! Image URL updated correctly.The container image URL correctly reflects the new naming convention (
extension-bootcinstead ofpodman-desktop-extension-bootc), aligning with the PR objectives..github/workflows/release.yaml (2)
106-147: Builder image workflow looks correct.The builder image job correctly builds multi-platform images, publishes to ghcr.io, and generates attestations. This will work properly once the job name issue is resolved.
193-206: Release job structure is correct.The release job properly depends on all prerequisite jobs and publishes the release. This will function correctly once the tag job outputs are declared.
.github/workflows/build-next.yaml (2)
45-89: Builder image job is well-structured.The conditional execution based on file changes is efficient, and the multi-platform build with attestation follows best practices.
90-95: Extension image dependency logic is intentional.The
if: always()combined withneeds: builder-imageallows the extension image to build using the previously published:nextbuilder image when no builder changes are detected. This supports the bootstrap approach described in the PR.build/Containerfile (1)
18-18: Verify the bootstrap process for the builder image.The Containerfile references
extension-bootc-builder:next, but this image won't exist on the first run. The PR description mentions keeping the root Containerfile temporarily for bootstrapping—confirm the bootstrap sequence is documented and tested.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/build-next.yaml (1)
36-44: Filename mismatch already flagged.This issue was identified in a previous review: line 39 references
.github/workflows/next-build.yamlbut the actual filename isbuild-next.yaml, preventing proper change detection for this workflow file.
🧹 Nitpick comments (1)
build/Containerfile.builder (1)
27-28: Consider adding npm cache cleanup to reduce image size.pnpm@10 is a valid, stable release (January 2025). To optimize the Docker image, consider adding npm cache cleanup after the global pnpm install:
-RUN npm install --global pnpm@10 && \ - pnpm --frozen-lockfile install +RUN npm install --global pnpm@10 && \ + npm cache clean --force && \ + pnpm --frozen-lockfile install
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build-next.yaml(2 hunks)build/Containerfile(1 hunks)build/Containerfile.builder(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- build/Containerfile
🧰 Additional context used
🪛 RuboCop (1.81.7)
build/Containerfile.builder
[fatal] 18-18: unexpected token tCOLON
(Using Ruby 2.7 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
⏰ 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). (4)
- GitHub Check: linter, formatter, and tests / ubuntu-22.04
- GitHub Check: e2e tests
- GitHub Check: linter, formatter, and tests / macos-14
- GitHub Check: linter, formatter, and tests / windows-2022
🔇 Additional comments (4)
.github/workflows/build-next.yaml (4)
2-2: LGTM!Copyright year correctly updated to 2023-2025.
90-94: Verify the if: always() logic with job dependency.The extension-image job has
if: always()combined withneeds: builder-image. This means:
- The job runs even if
builder-imagefails or is skipped- When
builder_required=false,builder-imageis skipped, butextension-imagestill runsConfirm this is the intended behavior - that extension images should be built even when the builder image is not rebuilt.
Based on the PR objectives, this appears intentional: the extension build can reuse the previous builder image when builder files haven't changed. However, verify that the extension build (via
build/Containerfile) can successfully pull and use the existing builder image whenbuilder-imageis skipped.You may want to add a comment explaining this behavior:
extension-image: name: Build and publish extension OCI image + # Always run to publish extension image, reusing existing builder image if not rebuilt if: always() runs-on: ubuntu-24.04 needs: builder-image
83-88: LGTM!Excellent addition of artifact attestations for both builder and extension images. This provides build provenance and enhances supply chain security.
Also applies to: 128-133
56-66: Verify containerfiles path and builder image reference.The builder-image job references
build/Containerfile.builderwhich is being added in this PR. Ensure that:
- The path is correct relative to the repository root.
- The Containerfile.builder is complete and functional for the builder image purpose.
The parameters look appropriate for a multi-platform build.
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| FROM registry.access.redhat.com/ubi10/nodejs-22-minimal:10.1-1764649415 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check image version information from registry metadata if available
# and search for Node.js 22 security status
# First, let's check if there's any documentation in the repo about why this version was chosen
echo "=== Checking for any documentation about image versioning ==="
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "Dockerfile*" -o -name "Containerfile*" \) -exec grep -l "nodejs-22\|ubi10.*nodejs\|10\.1-1764649415" {} \;
# Search for any version management files
echo -e "\n=== Checking for version pinning rationale ==="
find . -type f \( -name ".dockerignore" -o -name "*.env" -o -name "versions.txt" -o -name "VERSIONS" \)
# Check the Containerfile for any comments about version pinning
echo -e "\n=== Full Containerfile.builder content ==="
cat build/Containerfile.builder
# Check git history for when this version was pinned
echo -e "\n=== Recent git history for this file ==="
git log --oneline -10 build/Containerfile.builder 2>/dev/null || echo "Git history not available"Repository: podman-desktop/extension-bootc
Length of output: 204
🌐 Web query:
UBI10 nodejs-22-minimal image version 10.1 security status Red Hat
💡 Result:
Short answer: The UBI10 nodejs-22-minimal images for RHEL 10.1 have received multiple security updates in 2025 (Important/High) addressing several Node.js CVEs — you should pull the latest 10.1 nodejs-22-minimal image and apply those updates. See Red Hat’s RHEL 10.1 notes and the RHSA advisories for nodejs:22 (examples: RHSA-2025:8493, RHSA-2025:1613; additional RHSA advisories exist). [1][2][3]
References
- RHEL 10.1 release notes (nodejs/nodejs24 context). [1]
- RHSA-2025:8493 (nodejs:22 — RHEL 10 advisory / CVE list). [2]
- RHSA-2025:1613 (nodejs:22 — security fixes such as CVE-2025-22150, CVE-2025-23085, CVE-2025-23083). [3]
If you want, I can: (a) check the exact image digest/tag in the Red Hat Catalog for ubi10/nodejs-22-minimal:10.1, or (b) list the specific CVEs and RHSA IDs affecting the 10.1 build. Which would you prefer?
🌐 Web query:
Node.js 22 LTS security vulnerabilities 2025
💡 Result:
Short summary — Node.js 22.x (LTS) security issues in 2025
- CVE-2025-23166 — improper error handling in async crypto (SignTraits::DeriveBits) allowing remote process crash (High). Patch released in May 2025. [1][2]
- CVE-2025-23083 — diagnostics_channel exposes internal worker constructors affecting the Permission Model (--permission) (High). Published Jan 2025; patched in subsequent security releases. [3][4]
- CVE-2025-23084 (and follow-up CVE-2025-27210) — Windows drive/device-name handling in path APIs (path.join / path.normalize) allowing path-traversal bypass on Windows (Medium/High follow-up). Fixes released in Jan–July 2025. [5][6]
- CVE-2025-23165 — ReadFileUtf8 memory leak in fs internal binding that can lead to memory exhaustion (Low/DoS); patched in May 2025. [1][7]
- Additional related fixes in 2025 (e.g., V8 HashDoS / llhttp upgrades) affected active release lines including 22.x; see Node.js security posts for details. [1][6]
Actionable steps
- Upgrade to the latest patched 22.x release from the Node.js project (see the May/July 2025 security release notes). [1][6]
- Audit native/third-party native modules and update dependencies (especially crypto and diagnostics-related packages).
- If you run on Windows, pay special attention to path-related fixes and test path-handling code. [5][6]
Sources
[1] Node.js — May 14, 2025 Security Releases. (nodejs.org)
[2] Wiz vulnerability entry — CVE-2025-23166. (wiz.io)
[3] Ubuntu CVE page — CVE-2025-23083. (ubuntu.com)
[4] Wiz vulnerability entry — CVE-2025-23083. (wiz.io)
[5] Ubuntu CVE page — CVE-2025-23084 (path/join Windows). (ubuntu.com)
[6] Node.js — July 15, 2025 Security Releases (follow-up fixes). (nodejs.org)
[7] Coverage mentioning ReadFileUtf8 memory leak and release versions. (timber-dev.net)
If you want, I can: (A) check the exact latest 22.x patch version for your platform and provide upgrade commands, or (B) fetch the full Node.js release notes and CVE text. Which would you prefer?
Citations:
- 1: https://nodejs.org/ro/blog/vulnerability/may-2025-security-releases?utm_source=openai
- 2: https://www.wiz.io/vulnerability-database/cve/cve-2025-23166?utm_source=openai
- 3: https://ubuntu.com/security/CVE-2025-23083?utm_source=openai
- 4: https://www.wiz.io/vulnerability-database/cve/cve-2025-23083?utm_source=openai
- 5: https://ubuntu.com/security/CVE-2025-23084?utm_source=openai
- 6: https://nodejs.org/ro/blog/vulnerability/july-2025-security-releases?utm_source=openai
- 7: https://www.timber-dev.net/node-js-releases-may-2025-security-updates-addressing-critical-vulnerabilities/?utm_source=openai
Update the base image to the latest 10.1- build.*
The pinned version 10.1-1764649415 is outdated. Red Hat released multiple security updates for ubi10/nodejs-22-minimal throughout 2025 (RHSA-2025:8493, RHSA-2025:1613) addressing high-severity Node.js 22 CVEs including CVE-2025-23166 (async crypto), CVE-2025-23083 (Permission Model bypass), and CVE-2025-23084 (path traversal). Pull the latest ubi10/nodejs-22-minimal:10.1-* image to include these patches.
🧰 Tools
🪛 RuboCop (1.81.7)
[fatal] 18-18: unexpected token tCOLON
(Using Ruby 2.7 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
🤖 Prompt for AI Agents
In build/Containerfile.builder around line 18, the base image is pinned to an
outdated tag (10.1-1764649415); update that FROM line to reference the latest
10.1-* build (e.g., replace the fixed tag with the 10.1-* tag or a CI-driven
variable that resolves to the latest 10.1 image), rebuild the image so Docker
pulls the newest ubi10/nodejs-22-minimal:10.1-* manifest, and then run your
container image build and security scan to verify the updated image includes the
RHSA-2025 fixes before merging.
jeffmaury
left a comment
jeffmaury
left a comment
There was a problem hiding this comment.
We are recommanding to use commit id reference for actions and sha based image reference.
Also noticed other extensions do not use nodejs-minimal base image but this should be on their side to switch to it
There was a problem hiding this comment.
I tried to build this Containerfile.builder but I am seing the following error:
I tried to do a git clean -xdf
| - name: Check for builder-related changes | ||
| id: check | ||
| run: | | ||
| if git diff --name-only HEAD^ HEAD | grep -E '^(package.json|pnpm-lock.yaml|build/Containerfile.builder|.github/workflows/build-next.yaml)$'; then | ||
| echo "builder_required=true" >> $GITHUB_OUTPUT | ||
| else | ||
| echo "builder_required=false" >> $GITHUB_OUTPUT | ||
| fi |
There was a problem hiding this comment.
I'm a bit septic, are we sure we will not in a near future have this not covering all cases?
There was a problem hiding this comment.
Agreed, but following the pattern from Apple and RHEL extensions. So I'd prefer to defer due to 'existing pattern' and I think we need a 'build epic' for future sprint where someone would look at multi-stage build, sharing workflow jobs, etc.
| - name: Generate artifact attestation | ||
| uses: actions/attest-build-provenance@v3 | ||
| with: | ||
| subject-name: ghcr.io/${{ github.repository_owner }}/extension-bootc-builder | ||
| subject-digest: ${{ steps.push-to-ghcr.outputs.digest }} | ||
| push-to-registry: true |
| uses: redhat-actions/buildah-build@v2 | ||
| with: | ||
| image: extension-bootc | ||
| tags: next ${{ github.sha }} |
There was a problem hiding this comment.
this is a list of tags:
- next
- the sha of commit
| @@ -0,0 +1,28 @@ | |||
| # | |||
There was a problem hiding this comment.
out of the context of this PR, but we need to revise the usage of a builder container, multi stage container exists, and we could use caching mechanism to make them as fast, and avoid publishing a builder image
benoitf
left a comment
benoitf
left a comment
There was a problem hiding this comment.
build doesn't work
in builder, adding the line
# copy the application files to the /opt/app-root/extension-source directory
WORKDIR /opt/app-root/extension-source
+RUN mkdir -p /opt/app-root/extension-source
fixes the issue, but then the Containerfile is not working. Is it up-to-date ?
for example it tries to copy files that do not exist (as it's inside some subfolders) (like *.png, scripts, etc)
The final output of the build was an OCI container, but it was purely used for packaging. This moves the Containerfile to the /build folder and does the full pnpm build using a builder image. Images published are extension-bootc-builder and extension-bootc, switching from the previous podman-desktop-extension-bootc to match what newer extensions have done and make the switch obvious. First part: leaves existing container file in root for e2e tests to run, once workflow has successfully run once we can reuse the latest builder image instead, and remove the Containerfile at root. First part of podman-desktop#2101. Signed-off-by: Tim deBoer <git@tdeboer.ca>
deboer-tim
force-pushed
the
container-build
branch
from
a70c370 to
3623403
Compare
There was a problem hiding this comment.
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)
.github/workflows/release.yaml (1)
34-104: Critical: Missing job outputs and name mismatch.The workflow references
needs.tag.outputsthroughout (lines 112, 119, 164, 199, 206), but:
- No job named "tag" exists—only a job named "build" at line 34
- The "build" job doesn't define any outputs, so
githubTag,bootcExtensionVersion, andreleaseIdare unavailableThis will cause the workflow to fail.
Apply this diff to fix the issue:
jobs: - build: + tag: runs-on: ubuntu-22.04 + outputs: + githubTag: ${{ steps.TAG_UTIL.outputs.githubTag }} + bootcExtensionVersion: ${{ steps.TAG_UTIL.outputs.bootcExtensionVersion }} + releaseId: ${{ steps.create_release.outputs.id }} steps:
♻️ Duplicate comments (1)
build/Containerfile.builder (1)
18-18: Base image security concerns already flagged.The outdated base image version has already been flagged in previous review comments with security advisory details.
🧹 Nitpick comments (2)
build/Containerfile.builder (1)
24-25: Redundant directory creation.Line 24 sets WORKDIR to
/opt/app-root/extension-source, which automatically creates the directory if it doesn't exist. Line 25's explicitmkdir -pis redundant.Apply this diff to remove the redundant step:
WORKDIR /opt/app-root/extension-source -RUN mkdir -p /opt/app-root/extension-source COPY package.json pnpm-lock.yaml pnpm-workspace.yaml .npmrc /opt/app-root/extension-source/.github/workflows/build-next.yaml (1)
36-43: Change detection may miss relevant files.The builder change detection only checks:
package.json,pnpm-lock.yaml,build/Containerfile.builder,.github/workflows/build-next.yamlOther files that might affect the builder:
pnpm-workspace.yaml(workspace configuration).npmrc(npm configuration)packages/*/package.json(workspace package manifests)All these files are copied into the builder image (Containerfile.builder line 26-28).
Apply this diff to include all copied files in change detection:
- if git diff --name-only HEAD^ HEAD | grep -E '^(package.json|pnpm-lock.yaml|build/Containerfile.builder|.github/workflows/build-next.yaml)$'; then + if git diff --name-only HEAD^ HEAD | grep -E '^(package.json|pnpm-lock.yaml|pnpm-workspace.yaml|\.npmrc|packages/(backend|frontend)/package.json|build/Containerfile.builder|.github/workflows/build-next.yaml)$'; then echo "builder_required=true" >> $GITHUB_OUTPUT
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/build-next.yaml(2 hunks).github/workflows/release.yaml(2 hunks)RELEASE.md(1 hunks)build/Containerfile(1 hunks)build/Containerfile.builder(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- build/Containerfile
🧰 Additional context used
🪛 actionlint (1.7.9)
.github/workflows/release.yaml
106-106: job "builder-image" needs job "tag" which does not exist in this workflow
(job-needs)
112-112: property "tag" is not defined in object type {}
(expression)
119-119: property "tag" is not defined in object type {}
(expression)
148-148: job "extension-image" needs job "tag" which does not exist in this workflow
(job-needs)
164-164: property "tag" is not defined in object type {builder-image: {outputs: {}; result: string}}
(expression)
193-193: job "release" needs job "tag" which does not exist in this workflow
(job-needs)
199-199: property "tag" is not defined in object type {builder-image: {outputs: {}; result: string}; extension-image: {outputs: {}; result: string}}
(expression)
206-206: property "tag" is not defined in object type {builder-image: {outputs: {}; result: string}; extension-image: {outputs: {}; result: string}}
(expression)
🪛 markdownlint-cli2 (0.18.1)
RELEASE.md
22-22: Bare URL used
(MD034, no-bare-urls)
🪛 RuboCop (1.81.7)
build/Containerfile.builder
[fatal] 18-18: unexpected token tCOLON
(Using Ruby 2.7 parser; configure using TargetRubyVersion parameter, under AllCops)
(Lint/Syntax)
⏰ 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). (3)
- GitHub Check: linter, formatter, and tests / macos-14
- GitHub Check: linter, formatter, and tests / windows-2022
- GitHub Check: e2e tests
🔇 Additional comments (5)
RELEASE.md (1)
22-22: LGTM! Image URL updated correctly.The URL correctly reflects the new container naming scheme (extension-bootc instead of podman-desktop-extension-bootc) as described in the PR objectives.
Note: The bare URL could optionally be formatted as a Markdown link for better formatting, but this is fine as-is.
.github/workflows/release.yaml (1)
106-147: Verify builder image availability for release workflow.The builder-image job builds from scratch on every release. If the release workflow fails after the tag step, a retry would require rebuilding the builder image even though it hasn't changed.
Consider whether the release workflow should:
- Reuse the latest builder image from the CI workflow when possible
- Only rebuild the builder if Containerfile.builder or dependencies changed
This is consistent with the PR objective mentioning reusing the latest builder image after the first successful workflow run.
.github/workflows/build-next.yaml (2)
90-94: Clarify extension-image execution strategy.The extension-image job has
if: always()combined withneeds: builder-image. This means:
- If builder-image is skipped (builder not required): extension-image runs ✓
- If builder-image fails: extension-image still runs ✗
The second scenario may be problematic if the extension build depends on a successful builder.
Consider using
if: always() || needs.builder-image.result == 'success' || needs.builder-image.result == 'skipped'for more precise control, or clarify if running after builder failure is intentional.
83-88: LGTM! Artifact attestation for supply chain security.The build provenance attestation enhances supply chain security by creating verifiable metadata about how the image was built. This is a best practice for published container images.
build/Containerfile.builder (1)
30-31: Consider pinning pnpm version more precisely.While
pnpm@10provides major version pinning, using a more specific version (e.g.,pnpm@10.x.x) would ensure reproducible builds and prevent unexpected breaking changes within the v10 major release line.
| - uses: actions/checkout@v6 | ||
| with: | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
Inconsistent fetch-depth between jobs.
The extension-image job uses fetch-depth: 0 while builder-image doesn't specify it (defaults to 1). If both need full history for build context, they should be consistent.
Apply this diff if full history is needed for builder-image:
- uses: actions/checkout@v6
with:
ref: ${{ needs.tag.outputs.githubTag }}
+ fetch-depth: 0Otherwise, remove fetch-depth: 0 from extension-image if shallow clone is sufficient.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
.github/workflows/release.yaml around lines 155-157: inconsistent fetch-depth
between jobs — extension-image sets fetch-depth: 0 while builder-image uses the
default shallow clone; decide which behavior is required and make them
consistent: if both jobs need full history, add with: fetch-depth: 0 to the
builder-image checkout step; otherwise remove the with: fetch-depth: 0 block
from the extension-image checkout so both use the default shallow clone.
Sorry, this PR contained lots of crap, and I clearly hadn't run what I pushed locally. To top it off I applied suggestions from the automated PR review that actually broke it further. I've replaced most of the file copying sections and learned from what other extensions did, so I've force-pushed to start over. This commit successfully builds both images for me, and I've confirmed that the final contents match the last bootc release image. |
| # We require the macadam.js binaries and library, so we will manually copy this over to the container image. | ||
| # we rely on `pnpm build` before creating the container image, so we can safely assume that the macadam.js binaries are already present in the node_modules directory | ||
| # and can copy them over to the container image. | ||
| COPY node_modules/@crc-org/macadam.js /opt/app-root/extension/node_modules/@crc-org/macadam.js |
There was a problem hiding this comment.
question: I am a little bit concerned why those dependencies are handled manually and not through pnpm like the RHEL extension ?
There was a problem hiding this comment.
Me too, but this is unchanged from the current containerfile. As someone we know would say 'one PR one change'. :)
I see what the RHEL extension did and can replicate in a following PR.
There was a problem hiding this comment.
Not sure to understand as Containerfile(s) are new in this PR ?
There was a problem hiding this comment.
Not sure to understand as Containerfile(s) are new in this PR ?
There is an existing Containerfile at the root of the repository
There was a problem hiding this comment.
non-blocker, but a concern.
But any changes to the Containerfile that's outside-the-norm we should also check to see if it's correctly adding the arm64 madacam binary when building an arm64 container image.
Same for amd64, etc.
There were issues in the past where you would build this container image with podman build --platform linux/amd64 and because you are building it on macOS.. it adds the arm64 binary, etc.
benoitf
left a comment
benoitf
left a comment
There was a problem hiding this comment.
was able to build the image now and load the plug-in
maybe QE should validate (or validate the image that will be built on CI before cutting a tag)
7 participants
What does this PR do?
The final output of the build was an OCI container, but it was purely used for packaging. This moves the Containerfile to the /build folder and does the full pnpm build using a builder image.
Images published are extension-bootc-builder and extension-bootc, switching from the previous podman-desktop-extension-bootc to match what newer extensions have done and make the switch obvious.
First part: leaves existing container file in root for e2e tests to run, once workflow has successfully run once we can reuse the latest builder image instead, and remove the Containerfile at root.
Screenshot / video of UI
N/A
What issues does this PR fix or reference?
First part of #2101.
How to test this PR?
Merge, make sure builds complete and compare images.