Build OCI image to use CLI in containers#208
Conversation
📝 WalkthroughWalkthroughAdds multi-stage Dockerfiles, a reusable container build-and-publish workflow, a daily container cleanup workflow, and CI updates to upload the built CLI ZIP and invoke the reusable workflow to optionally build, tag, attest, and push multi-arch container images to GHCR. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as build.yml (caller)
participant Artifact as actions/upload-artifact
participant Reusable as container.yml (called workflow)
participant Checkout as actions/checkout
participant QEMU as qemu/setup
participant Buildx as docker/setup-buildx
participant Docker as docker/build-push-action
participant Registry as GHCR
participant Attest as actions/attest-build-provenance
CI->>Artifact: upload `build_zip` (./target/*oscal-cli.zip)
CI->>Reusable: workflow_call(push: true)
Reusable->>Checkout: checkout repo + submodules
Reusable->>Artifact: download `build_zip` to ./target
Reusable->>QEMU: setup QEMU (cross-arch)
Reusable->>Buildx: setup Buildx (amd64, arm64)
Reusable->>Docker: build image (Dockerfile.ci) with metadata/labels
alt push == true
Docker->>Registry: authenticate & push image (tags)
Reusable->>Attest: publish attestation for image digest
Attest->>Registry: store attestation
else
Docker-->>Reusable: build without push
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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. 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: 3
🧹 Nitpick comments (2)
Dockerfile (2)
13-13: PreferCOPYoverADDfor source tree (and ensure a.dockerignore).
ADD .can introduce surprises (and typically bloats context/can hurt caching).-ADD . /usr/local/src +COPY . /usr/local/src
50-54: Recommended hardening: run as non-root in the runtime image.
Not strictly required, but a good default for published images.FROM ${RUNNER_IMAGE} AS runner COPY --from=builder /opt/oscal-cli-extended /opt/oscal-cli-extended WORKDIR /opt/oscal-cli-extended +RUN addgroup -S oscal && adduser -S -G oscal oscal \ + && chown -R oscal:oscal /opt/oscal-cli-extended +USER oscal RUN /opt/oscal-cli-extended/bin/oscal-cli --version ENTRYPOINT [ "/opt/oscal-cli-extended/bin/oscal-cli" ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(1 hunks)
🧰 Additional context used
🪛 Hadolint (2.14.0)
Dockerfile
[error] 33-33: unexpected 'R'
expecting a new line followed by the next instruction
(DL1000)
⏰ 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: Website
- GitHub Check: Code
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yml(1 hunks)
⏰ 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: Website
- GitHub Check: Code
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
Dockerfile (1)
42-47: Remove unnecessarypom.xmlCOPY or document its purpose.Line 44 copies
pom.xmlto/tmp, but it is never referenced in the subsequent steps. Theunzipoperation on line 47 only needs the ZIP file. Ifpom.xmlis not required, remove it to reduce layer size and clarify intent.# If using a prebuilt ZIP, require a deterministic filename in build context. # (CI can place it there before `docker build`.) -COPY pom.xml /tmp/ COPY oscal-cli-extended.zip /tmp/If
pom.xmlserves a documented purpose (e.g., for build metadata or licensing), add a comment explaining it..github/workflows/build.yml (1)
119-122: Consider gating or removing verbose diagnostic commands.Lines 119–122 run
pwd,ls -lha, andfind .unconditionally before every build. Thefind .command will produce extremely verbose output on a large repository and clutter CI logs without corresponding benefit for automated builds.Remove these debug commands or gate them to manual/workflow_dispatch runs:
- - run: | - pwd - ls -lha - find '.' + # Optional: uncomment for debugging + # - run: | + # pwd + # ls -lha + # find . -type f -name '*.zip' | head -20Or, conditionally run only on manual dispatch:
- - run: | + - if: github.event_name == 'workflow_dispatch' + run: | pwd ls -lha find . -type f -name 'Dockerfile' -o -name 'oscal-cli-extended.zip'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(3 hunks)Dockerfile(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-13T15:28:58.145Z
Learnt from: david-waltermire
Repo: metaschema-framework/liboscal-java PR: 222
File: .github/workflows/build.yml:30-32
Timestamp: 2025-12-13T15:28:58.145Z
Learning: In the liboscal-java repository, nightly builds are managed independently using GitHub Actions schedule triggers (cron), not via repository_dispatch from metaschema-java. Each repo in the metaschema-framework manages its own nightly builds.
Applied to files:
.github/workflows/build.yml
🪛 Hadolint (2.14.0)
Dockerfile
[error] 33-33: unexpected 'R'
expecting a new line followed by the next instruction
(DL1000)
⏰ 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: Code
🔇 Additional comments (4)
.github/workflows/build.yml (3)
43-50: Approve: Container registry environment variables and Docker metadata flags are well-configured.The registry, image naming, and metadata environment variables are properly defined and follow Docker/OCI conventions. The
DOCKER_METADATA_PR_HEAD_SHAandDOCKER_METADATA_ANNOTATIONS_LEVELSflags are correct for PR head SHA tracking and manifest/index-level annotations.
59-61: Approve: New permissions are now justified by container build/push/attestation steps.The addition of
packages: write,attestations: write, andid-token: writepermissions is now justified by the container image build-push (line 160), login (line 131), and attestations steps (line 173). This follows the principle of least privilege correctly.
160-171: Verify: where does the prebuiltoscal-cli-extended.zipcome from before Docker build?The workflow sets
USE_PREBUILT_ZIP=yes(line 46) and passes it todocker build(line 165), which tells the Dockerfile to use the prebuilt path. However, the Docker build-push step (line 160) has no preceding step that creates or copiesoscal-cli-extended.zipinto the build context.The Maven build step (line 97) runs
mvn ... deploy, but the output artifact placement is not visible. Verify that:
- The Maven build produces
oscal-cli-extended.zip(or a file that is renamed to match).- Before the Docker build-push step, the zip is copied to the repository root or build context so that line 45 of the Dockerfile (
COPY oscal-cli-extended.zip /tmp/) succeeds.Run this to trace artifact generation:
#!/bin/bash # Check Maven pom.xml for the final artifact name and location echo "=== Searching pom.xml for artifact assembly configuration ===" fd pom.xml --exec grep -l "assembly\|finalName\|zip" {} \; 2>/dev/null | head -5 # Check if there's a step that moves the artifact before docker build echo -e "\n=== Checking for Maven artifact handling in workflow ===" sed -n '97,160p' .github/workflows/build.yml | grep -i "zip\|artifact\|target"If the Maven build produces the zip but it's not copied to the root, add a step before the Docker build to copy it:
- name: Perform CodeQL Analysis uses: github/codeql-action/analyze@1b168cd39490f61582a9beae412bb7057a6b2c4e with: upload: ${{ env.UPLOAD_CODEQL }} + - name: Prepare prebuilt ZIP for Docker build + run: | + if [ -f "target/oscal-cli-extended.zip" ]; then + cp target/oscal-cli-extended.zip . + echo "Copied prebuilt ZIP to build context" + else + echo "ERROR: oscal-cli-extended.zip not found in target/" + exit 1 + fi - run: |Dockerfile (1)
32-33: [Rewritten review comment]
[Classification tag]
|
Sigh, I will need to go back to the drawing board with this approach, it is more brittle than I thought. I will rubber duck debug with Claude and redesign it to be less brittle. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/build.yml (2)
122-126: CRITICAL: Fix YAML syntax error —uses:must precedewith:in step definition.Line 124 places
with:beforeuses:on line 126, violating the GitHub Actions step schema. The correct order isuses→with.This is a persistent issue from previous reviews and will cause the workflow to fail at parse time.
- name: Container image buildx setup for cross-arch builds id: image_setup_buildx + uses: docker/setup-buildx-action@988b5a0280414f521da01fcc63a27aeeb4b104db with: platforms: linux/amd64 - uses: docker/setup-buildx-action@988b5a0280414f521da01fcc63a27aeeb4b104db
168-174: CRITICAL: Attestation step condition references non-existentmainbranch in triggers.The conditional
if:statement (line 168) checksgithub.ref == 'refs/heads/main', but the workflow only triggers onrelease/**,develop, andfeature/**branches (lines 3–6, 10–12). Themainbranch never appears in the trigger configuration, so this condition is impossible to satisfy.This is a persistent logic error from previous reviews. Fix by aligning with actual triggers:
- - if: github.repository == env.HOME_REPO && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/develop' || startsWith(github.ref, 'refs/heads/feature')) + - if: github.repository == env.HOME_REPO && (github.ref == 'refs/heads/develop' || startsWith(github.ref, 'refs/heads/release/') || startsWith(github.ref, 'refs/heads/feature/'))Alternatively, if attestations should only run on release branches:
- - if: github.repository == env.HOME_REPO && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/develop' || startsWith(github.ref, 'refs/heads/feature')) + - if: github.repository == env.HOME_REPO && startsWith(github.ref, 'refs/heads/release/')Clarify the intent: should attestations run on all branches (develop, feature, release), or only on release branches?
🧹 Nitpick comments (3)
Dockerfile.ci (2)
1-7: Builder stage uses Maven image solely for unpacking — consider a lighter alternative.The builder stage pulls the entire Maven image (maven:3.9.9-eclipse-temurin-17-alpine ≈ 500+ MB) only to unzip a pre-built artifact. This adds unnecessary bloat to the build cache and layer history.
For a more efficient build, use a minimal Alpine image with
unzippre-installed, or a purpose-built unpacker base. The Maven image offers no benefit here since the CLI is already built by the Maven job (line 98 in build.yml).Consider this lighter alternative:
-ARG BUILDER_IMAGE=maven:3.9.9-eclipse-temurin-17-alpine +ARG BUILDER_IMAGE=alpine:latest ARG RUNNER_IMAGE=eclipse-temurin:17-alpine FROM ${BUILDER_IMAGE} AS builder +RUN apk add --no-cache unzip COPY ./target/*oscal-cli.zip /tmp/oscal-cli.zip WORKDIR /tmp RUN unzip /tmp/oscal-cli.zip -d /opt/oscal-cliThis reduces the builder image size and improves clarity of intent.
12-14: Verify binary integrity with explicit exit code check.The verification step (line 13) runs
oscal-cli --versionbut doesn't explicitly verify the exit code. If the binary is corrupted or missing, Docker build will not fail.Consider adding explicit failure on non-zero exit:
-RUN chmod +x /opt/oscal-cli/bin/oscal-cli && \ - /opt/oscal-cli/bin/oscal-cli --version +RUN chmod +x /opt/oscal-cli/bin/oscal-cli && \ + /opt/oscal-cli/bin/oscal-cli --version || exit 1Alternatively, structure as separate commands to aid debugging:
-RUN chmod +x /opt/oscal-cli/bin/oscal-cli && \ - /opt/oscal-cli/bin/oscal-cli --version +RUN chmod +x /opt/oscal-cli/bin/oscal-cli +RUN /opt/oscal-cli/bin/oscal-cli --versionThe second approach makes failures easier to diagnose in build logs.
.github/workflows/build.yml (1)
43-50: Remove unused environment variableUSE_PREBUILT_ZIP.Line 46 defines
USE_PREBUILT_ZIP: yes, but this variable is never referenced in the workflow. The Dockerfile.ci already assumes a pre-built ZIP at./target/*oscal-cli.zip(copied at build.yml line 5), so no conditional logic needs this flag.Per the principle of least surprise, remove unused variables:
REGISTRY: ghcr.io HOME_REPO: metaschema-framework/oscal-cli IMAGE_NAME: metaschema-framework/oscal-cli - USE_PREBUILT_ZIP: yes # Docs: github.com/docker/metadata-action/?tab=readme-ov-file#typesha
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(3 hunks)Dockerfile.ci(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-13T15:28:58.145Z
Learnt from: david-waltermire
Repo: metaschema-framework/liboscal-java PR: 222
File: .github/workflows/build.yml:30-32
Timestamp: 2025-12-13T15:28:58.145Z
Learning: In the liboscal-java repository, nightly builds are managed independently using GitHub Actions schedule triggers (cron), not via repository_dispatch from metaschema-java. Each repo in the metaschema-framework manages its own nightly builds.
Applied to files:
.github/workflows/build.yml
🔇 Additional comments (2)
.github/workflows/build.yml (2)
127-133: Container registry login is correctly configured.The login step properly uses the standard GITHUB_TOKEN secret for ghcr.io authentication and references environment variables for dynamic configuration.
134-156: Metadata and annotations configuration is comprehensive and follows OCI standards.The docker/metadata-action properly constructs image tags (SHA, branch, tag, PR refs) with latest flavor, and all OCI annotations are present (authors, docs, source, vendor, title, description, licenses). The use of environment variables for registry and image name enables dynamic configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Dockerfile.local (1)
27-34: Consider consolidating RUN instructions to reduce image layers.Multiple consecutive RUN instructions (apk add, mvn, find, mkdir, unzip, chmod) can be combined into fewer layers to reduce image size and improve cacheability. This is a recommended refactor for production images.
Apply this diff to consolidate build steps:
-RUN apk add --no-cache git unzip -RUN mvn -B -e -Prelease package -RUN find /usr/local/src/target \ - -iname '*oscal-cli.zip' \ - -exec cp {} /tmp/oscal-cli.zip \; -RUN mkdir -p /opt/oscal-cli -RUN unzip /tmp/oscal-cli.zip -d /opt/oscal-cli -RUN chmod +x /opt/oscal-cli/bin/oscal-cli +RUN apk add --no-cache git unzip && \ + mvn -B -e -Prelease package && \ + mkdir -p /opt/oscal-cli && \ + unzip /tmp/oscal-cli.zip -d /opt/oscal-cli && \ + chmod +x /opt/oscal-cli/bin/oscal-cliNote: The find command should be handled separately (see previous comment about error handling) before consolidating.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Dockerfile.ci(1 hunks)Dockerfile.local(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile.ci
🧰 Additional context used
🪛 Hadolint (2.14.0)
Dockerfile.local
[error] 27-27: unexpected 'R'
expecting a new line followed by the next instruction
(DL1000)
🔇 Additional comments (2)
Dockerfile.local (2)
29-31: Add error handling to the find command.The find command silently succeeds even if oscal-cli.zip is not found. If the file location changes or doesn't exist, the build continues and fails later at the unzip step (line 33) with a cryptic error, making debugging harder.
Apply this diff to fail fast if the file isn't found:
RUN find /usr/local/src/target \ -iname '*oscal-cli.zip' \ - -exec cp {} /tmp/oscal-cli.zip \; + -exec cp {} /tmp/oscal-cli.zip \; \ + || (echo "ERROR: oscal-cli.zip not found in target directory" && exit 1)Alternatively, use a more explicit approach:
-RUN find /usr/local/src/target \ - -iname '*oscal-cli.zip' \ - -exec cp {} /tmp/oscal-cli.zip \; +RUN set -e && \ + ZIPFILE=$(find /usr/local/src/target -iname '*oscal-cli.zip' | head -1) && \ + [ -n "$ZIPFILE" ] || (echo "ERROR: oscal-cli.zip not found" && exit 1) && \ + cp "$ZIPFILE" /tmp/oscal-cli.zip
28-28: Clarify test execution during Maven build.The Maven command
mvn -B -e -Prelease packagewill execute the project's test suite (unless configured otherwise in pom.xml). Depending on the test count, this could significantly extend build time. Consider whether tests should be skipped during container builds using-DskipTests, or if test execution is intentional for validation.
aj-stein
force-pushed
the
22-publish-docker-container
branch
from
8586521 to
8d7c76d
Compare
|
It's only been like a year @david-waltermire, but I did it since we're getting the band back together so to speak. Thanks for waiting for such a speedy PR on my part. 😆 |
|
The PR adds container building to the "build" workflow, which runs on Since this adds a good deal of complexity to the build workflow, should we move the container build into a seperate, parallel workflow? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/container.yml (1)
68-80: Consider adding SBOM generation and vulnerability scanning.While not blocking, consider enhancing the security posture by:
- Adding SBOM (Software Bill of Materials) generation using
docker/build-push-action's built-in support viasbom: true.- Adding container vulnerability scanning (e.g., using Trivy or similar tools) before publishing.
These additions would align with supply chain security best practices mentioned in the PR objectives regarding "verifiable, automation-friendly way to securely check the CLI."
🔎 Example SBOM addition
platforms: linux/amd64,linux/arm64/v8 cache-from: type=gha cache-to: type=gha,mode=max + sbom: true + provenance: true
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(1 hunks).github/workflows/container.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build.yml
🔇 Additional comments (9)
.github/workflows/container.yml (9)
1-9: LGTM! Well-structured reusable workflow.The workflow_call trigger with a boolean
pushinput provides good control over registry publishing behavior, with a safe default offalse.
10-16: LGTM! Environment variables are well-configured.The use of
DOCKER_METADATA_PR_HEAD_SHA: trueensures reproducible builds for PRs, and annotation levels are appropriate for multi-arch images.
18-26: LGTM! Permissions follow least-privilege principle.The job configuration uses ubuntu-24.04 and grants only the necessary permissions for building, publishing, and attesting container images.
31-33: LGTM! QEMU setup enables cross-architecture builds.Standard configuration for multi-arch container builds with properly pinned action.
39-45: LGTM! Standard GHCR authentication.The login step correctly uses
github.actorandGITHUB_TOKENfor authentication to GitHub Container Registry.
46-67: LGTM! Comprehensive metadata and tagging strategy.The metadata generation includes appropriate tags (SHA, branch, tag, PR) and follows OCI annotation standards with comprehensive project information.
27-30: Good checkout configuration with important caveatThe pinned hash,
submodules: recursive, andfilter: tree:0are all appropriate and well-supported. However, verify that Git LFS is not enabled (lfs: true) in this step, as it is incompatible with treeless clones.
34-38: Fix YAML syntax error:usesmust come beforewith.The GitHub Actions syntax requires the
useskeyword to appear before thewithkeyword. The current order will cause a workflow syntax error.🔎 Proposed fix
- name: Container image buildx setup for cross-arch builds id: image_setup_buildx + uses: docker/setup-buildx-action@988b5a0280414f521da01fcc63a27aeeb4b104db with: platforms: linux/amd64,linux/arm64/v8 - uses: docker/setup-buildx-action@988b5a0280414f521da01fcc63a27aeeb4b104dbLikely an incorrect or invalid review comment.
81-87: Setpush-to-registry: trueto align attestation storage with image deployment.When
inputs.pushis true, the container image is pushed to the registry, but the attestation defaults tofalse, storing it only in GitHub's Attestations API rather than the OCI registry. For consistency and to enable registry-level attestation verification (including Kubernetes admission controllers), attestations should be stored alongside the image.
|
Nevermind, I guess I take that back. lol |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Dockerfile.local (1)
9-9: Consider using COPY instead of ADD and adding .dockerignore.
ADDhas implicit tar extraction and URL fetching capabilities. For copying local directories,COPYis more explicit and preferred. Additionally, consider creating a.dockerignorefile to exclude unnecessary files (e.g.,.git,target,node_modules) from the build context.🔎 Suggested improvement
-ADD . /usr/local/src +COPY . /usr/local/srcAnd create a
.dockerignorefile at the repository root:.git target *.class *.jar .DS_Store
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Dockerfile.ci(1 hunks)Dockerfile.local(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile.ci
🧰 Additional context used
🪛 Hadolint (2.14.0)
Dockerfile.local
[error] 28-28: unexpected 'R'
expecting a new line followed by the next instruction
(DL1000)
⏰ 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: Code
🔇 Additional comments (3)
Dockerfile.local (3)
1-4: LGTM: BuildKit directive and base image setup are correct.The BuildKit syntax directive is properly placed, and the base images are appropriately pinned to specific versions.
37-41: LGTM: Runner stage is clean and well-structured.The runner stage correctly:
- Uses a minimal base image
- Copies only the necessary artifacts from the builder
- Validates the CLI binary with a version check
- Sets an appropriate entrypoint for container usage
29-35: Code implementation is correct as-is.Dockerfile.local properly builds the project from source using Maven (line 29), making it self-contained for local development. Dockerfile.ci, as expected, receives pre-built artifacts from the CI pipeline and simply extracts and installs them. This separation of concerns is the intended design—each Dockerfile serves its purpose correctly.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Dockerfile.ci (2)
6-6: Remove unnecessary git package.The
gitpackage is installed but never used in the builder stage. The builder only unpacks the ZIP artifact and doesn't perform any Git operations. Removing it will reduce the build time and intermediate layer size.🔎 Proposed fix (after fixing the package manager issue)
-RUN apt-get update && apt-get install -y --no-install-recommends git unzip && rm -rf /var/lib/apt/lists/* +RUN apt-get update && apt-get install -y --no-install-recommends unzip && rm -rf /var/lib/apt/lists/*
2-3: Update base image versions to latest stable releases.Replace
maven:3.9.9-eclipse-temurin-17withmaven:3.9.12-eclipse-temurin-17(or at minimummaven:3.9.10-eclipse-temurin-17) and use an explicit Eclipse Temurin 17 version tag such aseclipse-temurin:17.0.17instead of the untaggedeclipse-temurin:17to ensure consistent, secure builds with known CVE patches applied.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile.ci(1 hunks)
⏰ 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: Code
|
Ok, now it is ready for review @david-waltermire. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Dockerfile.local (1)
11-11: Clean up apt cache to reduce image size.The
apt-get installcommand should include--no-install-recommendsand remove cached package lists to follow Docker best practices and minimize the builder layer size.🔎 Proposed fix
-RUN apt-get update && apt-get install -y unzip +RUN apt-get update && apt-get install -y --no-install-recommends unzip \ + && rm -rf /var/lib/apt/lists/*.github/workflows/container.yml (1)
90-91: Inconsistent template spacing.Line 91 is missing a space before the closing braces in
${{ env.IMAGE_NAME}}. While this may still parse correctly, it's inconsistent with the spacing used elsewhere (e.g., line 90:${{ env.REGISTRY }}).🔎 Proposed fix
- subject-name: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME}} + subject-name: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/container.yml(1 hunks)Dockerfile.ci(1 hunks)Dockerfile.local(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile.ci
🧰 Additional context used
🪛 Hadolint (2.14.0)
Dockerfile.local
[error] 30-30: unexpected 'R'
expecting a new line followed by the next instruction
(DL1000)
⏰ 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: Code
🔇 Additional comments (3)
Dockerfile.local (2)
30-30: Hadolint DL1000 is a false positive.The static analysis tool reports an "unexpected 'R'" error on this line, but this is a false positive. The heredoc (lines 12-29) correctly terminates with
EOFon line 29, and line 30 is a validRUNinstruction. This error likely stems from the linter's incomplete understanding of BuildKit heredoc syntax.
38-46: LGTM: Security and verification practices are solid.The runner stage implements good practices by creating a dedicated non-root user (oscalcli), restricting privileges, and verifying the installation with a version check before setting the entrypoint.
.github/workflows/container.yml (1)
51-72: LGTM: Comprehensive OCI metadata annotations.The metadata configuration provides thorough OCI-compliant annotations including maintainers, documentation, source, vendor, and licensing information. The tagging strategy (SHA, branch, tag, PR) covers the necessary deployment scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Dockerfile.local (1)
11-11: Consider cleaning up apt lists to reduce builder image size.While this is a builder stage and the runner stage won't inherit these layers, cleaning up apt cache reduces local storage and registry cache size.
🔎 Suggested cleanup
-RUN apt-get update && apt-get install -y unzip +RUN apt-get update && apt-get install -y --no-install-recommends unzip && rm -rf /var/lib/apt/lists/*Dockerfile.ci (1)
6-6: Consider cleaning up apt lists to reduce builder image size.Similar to Dockerfile.local, adding cleanup reduces cache and storage footprint.
🔎 Suggested cleanup
-RUN apt-get update && apt-get install -y unzip +RUN apt-get update && apt-get install -y --no-install-recommends unzip && rm -rf /var/lib/apt/lists/*
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/container.yml(1 hunks)Dockerfile.ci(1 hunks)Dockerfile.local(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-13T15:29:03.112Z
Learnt from: david-waltermire
Repo: metaschema-framework/liboscal-java PR: 222
File: .github/workflows/build.yml:30-32
Timestamp: 2025-12-13T15:29:03.112Z
Learning: In the liboscal-java repository, nightly builds are managed independently using GitHub Actions schedule triggers (cron), not via repository_dispatch from metaschema-java. Each repo in the metaschema-framework manages its own nightly builds.
Applied to files:
Dockerfile.ci.github/workflows/container.yml
📚 Learning: 2025-12-17T13:27:43.679Z
Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.679Z
Learning: All changes require PR review with CODEOWNERS enforcement
Applied to files:
Dockerfile.ci
🪛 Hadolint (2.14.0)
Dockerfile.local
[error] 30-30: unexpected 'R'
expecting a new line followed by the next instruction
(DL1000)
⏰ 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: Code
🔇 Additional comments (6)
Dockerfile.local (1)
38-48: LGTM! Excellent security posture.The runner stage correctly implements security best practices: non-root user with restricted shell, proper ownership of application directory, separate working directory for user mounts, and verification that runs as the non-root user before the container starts.
Dockerfile.ci (1)
12-21: LGTM! Consistent security implementation.The runner stage mirrors Dockerfile.local with the same secure non-root setup. The past review feedback on non-root user, correct package manager, and metadata has been properly addressed.
.github/workflows/container.yml (4)
1-16: LGTM! Well-structured reusable workflow.The workflow_call pattern with the push input provides good flexibility for CI/CD. Environment variables are properly scoped, and the Docker metadata configuration flags (PR_HEAD_SHA, ANNOTATIONS_LEVELS) follow current best practices.
17-43: LGTM! Proper permissions and multi-arch setup.The job configuration follows security best practices with appropriate least-privilege permissions. The multi-architecture build setup (QEMU + Buildx for amd64 and arm64/v8) is correctly configured, and the artifact download step properly retrieves the pre-built CLI zip.
51-72: LGTM! Comprehensive container metadata.The metadata configuration is thorough and follows best practices:
- SHA-based tags for immutability
- Ref-based tags for readability
latesttag restricted to main branch (per commit message intent)- Complete OCI annotations with proper maintainer info, documentation URLs, and CC0-1.0 license
86-92: Verify attestation publishing strategy.Line 92 sets
push-to-registry: falsefor build provenance attestations. This means attestations are generated but not published to the registry alongside the image.If you want users to be able to verify the image provenance (e.g., via
docker buildx imagetools inspectorcosign verify-attestation), consider settingpush-to-registry: trueto publish attestations to GHCR alongside the image.Current behavior:
- Attestations are generated and stored in GitHub Actions artifacts/logs
- Users cannot verify provenance directly from the registry
Alternative (if desired):
push-to-registry: false + push-to-registry: trueThis would enable end-users to verify supply chain provenance for published images.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
73-81: Consider documenting available version tags and multi-architecture support.The documentation shows only the
latesttag but doesn't explain the tagging strategy (stable releases, pre-releases, version-specific tags, etc.). Since the AI summary indicates multi-architecture images are built, it would be helpful to mention that users can run the container on different architectures without additional steps.🔎 Example enhancement
You might consider expanding the documentation like this (optional):
### Installing pre-built package as a container image -You can also download pre-release and official release versions of the CLI as an OCI-conformant image with `docker`, `podman`, or other tools. +You can also download pre-release and official release versions of the CLI as an OCI-conformant image with `docker`, `podman`, or other tools. Images are published with version tags (e.g., `v1.0.0`) and the `latest` tag for the most recent release. Multi-architecture images are available and automatically selected for your platform. \`\`\`sh docker pull ghcr.io/metaschema-framework/oscal-cli:latest -docker run -it ghcr.io/metaschema-framework/oscal-cli:latest --version +docker run ghcr.io/metaschema-framework/oscal-cli:latest --version \`\`\`
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md
⏰ 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: Code
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/container-cleanup.yml:
- Around line 22-30: Add a permissions block to the workflow with "packages:
write" and ensure the token used can manage GHCR packages: either grant the
org-level package "Manage Actions access" to this repository (so GITHUB_TOKEN
can delete the images) or replace token: ${{ secrets.GITHUB_TOKEN }} with a
repository secret holding a PAT that has write:packages and delete:packages
(e.g. token: ${{ secrets.PACKAGES_TOKEN }}). Ensure the new permissions block
(permissions: packages: write) is added at workflow root and update the token
reference in the container-cleanup job if using a PAT.
🧹 Nitpick comments (2)
Dockerfile.local (1)
31-36: Consider adding error handling for the find command.The
find ... -exec cpwill exit successfully even if no matching file is found, potentially causing confusing failures later when the zip doesn't exist.🔎 Suggested improvement
-RUN find /usr/local/src/target \ - -iname '*oscal-cli.zip' \ - -exec cp {} /tmp/oscal-cli.zip \; +RUN find /usr/local/src/target \ + -iname '*oscal-cli.zip' \ + -exec cp {} /tmp/oscal-cli.zip \; && \ + test -f /tmp/oscal-cli.zip.github/workflows/container-cleanup.yml (1)
25-27: Consider documenting the retention policy rationale.The retention settings (7-day cutoff, keep 5 most recent, exclude
latestand versioned tags) are reasonable, but adding inline comments would help future maintainers understand the tradeoff between storage costs and image availability.📝 Example with documentation comments
with: account: metaschema-framework image-names: oscal-cli + # Keep images from the last week to support recent development cut-off: 7d + # Retain at least 5 images even if older than 7d for rollback capability keep-n-most-recent: 5 + # Preserve release versions (v*) and the latest tag indefinitely image-tags: "!latest !v*" tag-selection: both
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/dependabot.yml.github/workflows/build.yml.github/workflows/container-cleanup.yml.github/workflows/container.ymlDockerfile.ciDockerfile.local
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/container.yml
- Dockerfile.ci
- .github/workflows/build.yml
🧰 Additional context used
🪛 Hadolint (2.14.0)
Dockerfile.local
[error] 30-30: unexpected 'R'
expecting a new line followed by the next instruction
(DL1000)
⏰ 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: Code
🔇 Additional comments (5)
.github/dependabot.yml (1)
16-21: LGTM!The Docker ecosystem configuration follows the established pattern of the existing entries (daily interval, develop target branch) and correctly points to the repository root where the Dockerfiles reside.
Dockerfile.local (3)
1-10: LGTM!The BuildKit syntax directive enables heredoc support, and the parameterized base images via ARGs provide good flexibility for local customization.
11-29: Correct package manager and valid heredoc syntax.The
apt-getusage is correct for the Debian-based Maven image. The Hadolint DL1000 error on line 30 is a false positive—Hadolint doesn't fully support BuildKit heredoc syntax, which is properly enabled by the# syntax=docker/dockerfile:1directive.Optionally, you could add
--no-install-recommendsand cleanup the apt cache (rm -rf /var/lib/apt/lists/*) on line 11, but since this is a builder stage that isn't persisted, the impact is minimal.
38-49: Good security practices in the runner stage.The non-root user setup with proper group/user creation, ownership assignment, and the build-time version verification are solid practices. The
/appworking directory provides a clean mount point for users..github/workflows/container-cleanup.yml (1)
21-21: The action SHA is valid and correctly pinned to v3.0.1.The pinned commit SHA
3b0972b2276b171b212f8c4efbca59ebba26ecebis verified as the official v3.0.1 release from September 23, 2025, and represents a sound security practice. The version remains reasonably current with no newer releases available; recent commits in the repository contain only dependency updates.
aj-stein
left a comment
aj-stein
left a comment
There was a problem hiding this comment.
I can't directly approve as an author, @david-waltermire. It looks good to me, please merge.
I was unable to quickly see some context on mobile when I looked earlier. I look forward to using the container work, so 🚀 🚢 !
- Update Docker action versions to latest - Add chown for /app directory in Dockerfiles - Add container-cleanup.yml workflow for image pruning - Add docker ecosystem to dependabot.yml - Skip container build for fork PRs (no registry access)
The -it flags allocate an interactive terminal which is unnecessary for the --version command that executes and exits immediately.
aj-stein
force-pushed
the
22-publish-docker-container
branch
from
0421b0a to
50e6ede
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/container.yml:
- Around line 62-63: The current condition setting latest (the line starting
with latest= and currently comparing github.ref to refs/heads/main) is
unreachable because builds never run on main; update that expression to match
your intended tagging strategy—e.g., set it to check refs/heads/develop for a
rolling latest from develop, use startsWith(github.ref, 'refs/heads/release/')
for release branches, use startsWith(github.ref, 'refs/tags/v') for version
tags, or alternatively add main to the build triggers in build.yml; locate the
existing latest=... entry in .github/workflows/container.yml and replace the
condition accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/dependabot.yml.github/workflows/build.yml.github/workflows/container-cleanup.yml.github/workflows/container.ymlDockerfile.ciDockerfile.localREADME.md
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/container-cleanup.yml
- .github/dependabot.yml
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2026-01-05T16:33:59.206Z
Learnt from: david-waltermire
Repo: metaschema-framework/metaschema-java PR: 0
File: :0-0
Timestamp: 2026-01-05T16:33:59.206Z
Learning: In metaschema-framework/metaschema-java, the core/metaschema submodule is owned by this project's maintainers; updates to it are coordinated within this repository, so no follow-up issues should be opened in external repositories for submodule changes.
Applied to files:
.github/workflows/container.ymlDockerfile.ci
📚 Learning: 2025-12-13T15:29:03.112Z
Learnt from: david-waltermire
Repo: metaschema-framework/liboscal-java PR: 222
File: .github/workflows/build.yml:30-32
Timestamp: 2025-12-13T15:29:03.112Z
Learning: In the liboscal-java repository, nightly builds are managed independently using GitHub Actions schedule triggers (cron), not via repository_dispatch from metaschema-java. Each repo in the metaschema-framework manages its own nightly builds.
Applied to files:
.github/workflows/container.ymlDockerfile.ci
📚 Learning: 2026-01-07T04:40:01.060Z
Learnt from: david-waltermire
Repo: metaschema-framework/metaschema-java PR: 623
File: .github/workflows/build.yml:8-14
Timestamp: 2026-01-07T04:40:01.060Z
Learning: In the metaschema-java repository, the build workflow intentionally includes `main` in the `pull_request.branches` trigger even though CONTRIBUTING.md requires PRs to target `develop`. This is a defensive design to provide feedback if someone accidentally targets main, rather than silent failure. The UPLOAD_SCAN_SARIF logic also needs main for security scan results.
Applied to files:
.github/workflows/build.yml
📚 Learning: 2026-01-07T17:36:32.578Z
Learnt from: david-waltermire
Repo: metaschema-framework/oscal-cli PR: 0
File: :0-0
Timestamp: 2026-01-07T17:36:32.578Z
Learning: In metaschema-framework repositories, nightly builds skip security scans (CodeQL and Trivy) because: (1) security scans run on every PR and push to main/develop/release branches providing continuous coverage, (2) GitHub Dependabot provides real-time alerts for dependency vulnerabilities between PRs, and (3) nightly builds focus on catching integration issues with upstream dependencies rather than security vulnerabilities. This is an intentional optimization that maintains security posture.
Applied to files:
.github/workflows/build.yml
📚 Learning: 2025-12-17T13:27:43.679Z
Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.679Z
Learning: All PRs require passing CI checks before merge
Applied to files:
.github/workflows/build.yml
📚 Learning: 2025-12-17T13:27:43.679Z
Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.679Z
Learning: All changes require PR review with CODEOWNERS enforcement
Applied to files:
.github/workflows/build.ymlDockerfile.ci
📚 Learning: 2025-12-17T13:27:43.679Z
Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.679Z
Learning: All PRs must be created from a personal fork and must target the develop branch (BLOCKING - required by CONTRIBUTING.md)
Applied to files:
.github/workflows/build.yml
📚 Learning: 2025-12-17T13:27:43.679Z
Learnt from: CR
Repo: metaschema-framework/metaschema-java PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-17T13:27:43.679Z
Learning: Squash non-relevant commits before submitting PR (BLOCKING)
Applied to files:
.github/workflows/build.yml
🪛 Hadolint (2.14.0)
Dockerfile.local
[error] 30-30: unexpected 'R'
expecting a new line followed by the next instruction
(DL1000)
⏰ 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: Code
🔇 Additional comments (7)
README.md (1)
73-81: LGTM! Clear container installation documentation.The new documentation section provides straightforward instructions for using the CLI as a container image. The commands follow Docker best practices, and the placement before the "Running" section is logical.
Dockerfile.local (1)
1-49: Clarify the purpose and usage of Dockerfile.local.This Dockerfile appears technically sound (all past issues have been addressed), but based on your earlier comment "No need for a build container we are not saving it," it's unclear whether this file is actually needed.
The PR uses
Dockerfile.cifor the CI/CD pipeline, so the purpose ofDockerfile.localis ambiguous. If it's intended for local development testing, please document this in:
- A comment at the top of the file
- The README's "Building" section
If it's not needed, consider removing it to reduce maintenance burden.
Note: The Hadolint DL1000 error on line 30 is a false positive—the RUN command is valid, and Hadolint is misinterpreting the heredoc syntax.
.github/workflows/build.yml (2)
108-112: LGTM! Artifact upload correctly configured.The build zip upload step properly packages the Maven build output for consumption by the container workflow. The artifact name
build_zipmatches the download step incontainer.yml(line 34).
113-125: LGTM! Container job properly configured with security controls.The build-container job correctly:
- Skips for fork PRs (preventing unauthorized registry access)
- Declares appropriate permissions for container operations
- Depends on build-code to ensure artifacts are available
- Uses the reusable container workflow with push enabled
Dockerfile.ci (1)
1-22: LGTM! Well-structured multi-stage build with security best practices.The Dockerfile.ci correctly implements:
- BuildKit syntax for advanced features
- Minimal builder stage for extracting the pre-built CLI
- Security-hardened runner stage with non-root user (oscalcli)
- Runtime verification step before finalizing the image
All previous review feedback has been incorporated. The image is production-ready.
.github/workflows/container.yml (2)
73-85: LGTM! Build and push configuration is correct.The build-push step properly:
- Guards registry push with
inputs.pushconditional- Builds for both linux/amd64 and linux/arm64/v8 (matching buildx configuration on line 43)
- References the correct Dockerfile (
Dockerfile.ci)- Uses metadata outputs for tags and labels
- Leverages GitHub Actions cache for faster subsequent builds
86-92: LGTM! Attestation configuration follows best practices.The build provenance attestation correctly:
- Runs only when images are pushed (conditional on
inputs.push)- References the built image via subject-name and digest
- Pushes attestation to the registry for verification
This provides supply chain security via SLSA provenance.
2 participants
Committer Notes
Closes #22.
All Submissions:
By submitting a pull request, you are agreeing to provide this contribution under the CC0 1.0 Universal public domain dedication.
Changes to Core Features:
Have you written new tests for your core changes, as applicable?Have you included examples of how to use your new feature(s)?Have you updated all website and readme documentation affected by the changes you made?Summary by CodeRabbit
New Features
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.