Conversation
|
Review these changes at https://app.gitnotebooks.com/AlphaSphereDotAI/vocalizr/pull/932 |
Reviewer's GuideIntroduce Apko and Melange configuration files to standardize container image creation and packaging, including environment setup, non-root execution, and a build pipeline for the Minicli application. Class diagram for Apko and Melange configuration entitiesclassDiagram
class ApkoConfig {
+keyring: list
+repositories: list
+packages: list
+entrypoint: command
+archs: list
+environment: map
+accounts: groups, users, run-as
+work-dir: string
+paths: list
}
class MelangeConfig {
+package: name, version, description, target-architecture, copyright, dependencies
+environment: contents
+pipeline: list
}
ApkoConfig "1" -- "1" MelangeConfig : uses
MelangeConfig o-- "Build Minicli application" PipelineStep
class PipelineStep {
+name: string
+runs: script
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds Melange and APKO packaging artifacts, updates CI to build and publish APKs via Melange/APKO instead of Docker, and expands .gitignore with additional artifact patterns. Includes new Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant M as Melange
participant UV as UV Toolchain
participant APKO as APKO (apko-publish)
participant Reg as Registry
GH->>M: Trigger `build_apk` job
Note right of M `#DFF7E6`: Melange runs build pipeline (env, keyring)
M->>UV: `uv tool install --no-cache vocalizr` + download spaCy model
UV-->>M: Produce APK artifact + signing key
M-->>GH: Upload APK artifact & Melange artifacts
GH->>APKO: Invoke `apko-publish` with APK and .apko.yaml
Note right of APKO `#FFF3D9`: APKO publishes image and emits digest/provenance
APKO->>Reg: Push image + provenance (APKO digest)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
Summary of ChangesHello @MH0386, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request modernizes the container creation process by integrating "apko" and "melange". It introduces new configuration files ("apko.yaml" and "melange.yaml") to declaratively define the "vocalizr" application's container image and its underlying package, including build steps, dependencies, and secure user configurations. This change aims to streamline and enhance the reproducibility of the build pipeline. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Here's the code health analysis summary for commits Analysis Summary
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Your apko.yaml entrypoint is set to /usr/bin/vocalizr but the melange pipeline only builds and installs a
miniclibinary—either adjust the build to producevocalizror update the entrypoint to matchminicli. - You add
/home/nonroot/.local/binto PATH in apko.yaml but never create that directory—consider creating it (and setting ownership) in your build or work-dir step so user‐installed binaries will be found. - To ensure reproducible builds, consider pinning critical package versions (e.g.
uv,build-base, etc.) in both apko.yaml and melange.yaml rather than always pulling the latest.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Your apko.yaml entrypoint is set to /usr/bin/vocalizr but the melange pipeline only builds and installs a `minicli` binary—either adjust the build to produce `vocalizr` or update the entrypoint to match `minicli`.
- You add `/home/nonroot/.local/bin` to PATH in apko.yaml but never create that directory—consider creating it (and setting ownership) in your build or work-dir step so user‐installed binaries will be found.
- To ensure reproducible builds, consider pinning critical package versions (e.g. `uv`, `build-base`, etc.) in both apko.yaml and melange.yaml rather than always pulling the latest.
## Individual Comments
### Comment 1
<location> `melange.yaml:4` </location>
<code_context>
+package:
+ name: vocalizr
+ version: 0.1.0
+ description: Voice Generator part of the Chatacter Backend
+ target-architecture:
+ - all
</code_context>
<issue_to_address>
**suggestion (typo):** Typo in 'Chatacter' should be corrected to 'Character'.
Fixing the typo ensures the package description is clear and professional.
```suggestion
description: Voice Generator part of the Character Backend
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces apko.yaml and melange.yaml for building and packaging the vocalizr service. While the intention to automate container creation is clear, there are critical inconsistencies between the defined project name (vocalizr) and the artifacts being built (minicli) in the melange.yaml. Additionally, an irrelevant composer.json file is included in the build process. These issues need to be addressed to ensure the correct application is built and deployed.
| - ca-certificates-bundle | ||
| - wolfi-baselayout | ||
| entrypoint: | ||
| command: /usr/bin/vocalizr |
There was a problem hiding this comment.
The entrypoint specifies /usr/bin/vocalizr. However, the melange.yaml pipeline appears to be building and installing an executable named minicli into /usr/bin. This creates a critical mismatch, as the container will attempt to execute vocalizr but will likely find only minicli.
| - name: Build Minicli application | ||
| runs: |- | ||
| MINICLI_HOME="${{targets.destdir}}/usr/share/minicli" | ||
| EXEC_DIR="${{targets.destdir}}/usr/bin" | ||
| mkdir -p "${MINICLI_HOME}" "${EXEC_DIR}" | ||
| cp ./composer.json "${MINICLI_HOME}" | ||
| /usr/bin/uv install -d "${MINICLI_HOME}" --no-dev | ||
| cp ./minicli "${EXEC_DIR}" | ||
| chmod +x "${EXEC_DIR}/minicli" |
There was a problem hiding this comment.
The pipeline is named "Build Minicli application" and installs minicli as the executable. This is inconsistent with the package.name being vocalizr and the apko.yaml entrypoint expecting /usr/bin/vocalizr. This is a critical discrepancy. Please ensure that the melange.yaml correctly builds and installs the vocalizr application, not minicli, or clarify if minicli is an intended component of vocalizr and how it relates to the vocalizr entrypoint.
| MINICLI_HOME="${{targets.destdir}}/usr/share/minicli" | ||
| EXEC_DIR="${{targets.destdir}}/usr/bin" | ||
| mkdir -p "${MINICLI_HOME}" "${EXEC_DIR}" | ||
| cp ./composer.json "${MINICLI_HOME}" |
There was a problem hiding this comment.
The build pipeline copies composer.json into the MINICLI_HOME directory. composer.json is a dependency management file primarily used in PHP projects. Given that pyproject.toml is present and uv is used for Python package installation, including composer.json seems unnecessary and potentially incorrect for a Python application. Please confirm if this file is actually needed for the vocalizr Python service.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces apko and melange configurations to replace the existing Dockerfile-based container creation approach. The changes aim to use Chainguard's apko and melange tools for building the vocalizr voice generation application container.
Key changes:
- Adds
melange.yamlfor building the vocalizr package - Adds
apko.yamlfor container image configuration - Configures non-root user setup and environment variables for the Gradio application
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| melange.yaml | Defines package build configuration with dependencies and build pipeline |
| apko.yaml | Specifies container image structure, entrypoint, and runtime configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Build Minicli application | ||
| runs: |- | ||
| MINICLI_HOME="${{targets.destdir}}/usr/share/minicli" | ||
| EXEC_DIR="${{targets.destdir}}/usr/bin" | ||
| mkdir -p "${MINICLI_HOME}" "${EXEC_DIR}" | ||
| cp ./composer.json "${MINICLI_HOME}" | ||
| /usr/bin/uv install -d "${MINICLI_HOME}" --no-dev | ||
| cp ./minicli "${EXEC_DIR}" | ||
| chmod +x "${EXEC_DIR}/minicli" |
There was a problem hiding this comment.
The pipeline references PHP/Minicli-specific files and paths (composer.json, ./minicli) that don't exist in this Python project. This should be building vocalizr using uv, not minicli. The build steps should install Python dependencies and the vocalizr package instead.
| - name: Build Minicli application | |
| runs: |- | |
| MINICLI_HOME="${{targets.destdir}}/usr/share/minicli" | |
| EXEC_DIR="${{targets.destdir}}/usr/bin" | |
| mkdir -p "${MINICLI_HOME}" "${EXEC_DIR}" | |
| cp ./composer.json "${MINICLI_HOME}" | |
| /usr/bin/uv install -d "${MINICLI_HOME}" --no-dev | |
| cp ./minicli "${EXEC_DIR}" | |
| chmod +x "${EXEC_DIR}/minicli" | |
| - name: Build vocalizr Python package | |
| runs: |- | |
| # Install Python dependencies and the vocalizr package using uv | |
| /usr/bin/uv pip install . --system |
| entrypoint: | ||
| command: /usr/bin/vocalizr | ||
| archs: | ||
| - amd64 |
There was a problem hiding this comment.
[nitpick] The architecture is limited to 'amd64' only, but the melange.yaml specifies 'all' architectures. These should be consistent. Consider supporting arm64 as well for broader platform compatibility.
| - amd64 | |
| - amd64 | |
| - arm64 |
🧪 CI InsightsHere's what we observed from your CI run for 4f9d4ff. 🟢 All jobs passed!But CI Insights is watching 👀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
melange.yaml (1)
4-4: Fix typo in package description.Line 4 contains a typo: "Chatacter" should be "Character".
- description: Voice Generator part of the Chatacter Backend + description: Voice Generator part of the Character Backend
🧹 Nitpick comments (1)
.apko.yaml (1)
12-12: Consider adding arm64 architecture support.The
archssection restricts the image toamd64only. Past reviews note thatmelange.yamlsupports all architectures. For consistency and broader platform compatibility (especially for modern ARM-based systems), consider addingarm64support.archs: - amd64 + - arm64
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.apko.yaml(1 hunks)melange.yaml(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). (5)
- GitHub Check: Lint / Check
- GitHub Check: Test Image / Build APK with Melange
- GitHub Check: Agent
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (1)
melange.yaml (1)
39-40: Clarify source location and verify uv tool install will succeed.Line 39 uses
uv tool install ${{package.name}}to install vocalizr. In a build context, this attempts to fetch the package from PyPI. However, the pipeline does not include agit-checkoutstep to provide the source code, nor is vocalizr necessarily published to PyPI.The Docker Scout report shows a successful 4.7GB image build, suggesting this is working in practice. However, clarify:
- How is the vocalizr source code made available to the build pipeline?
- Is vocalizr published to PyPI at version 0.0.13?
- Should this be
uv tool install .(local source) instead?If the source should come from a local directory, apply:
- uv tool install ${{package.name}} + uv tool install .Or if explicitly pulling from a repository, use:
- uv tool install ${{package.name}} + uv tool install git+https://github.com/AlphaSphereDotAI/vocalizr.git@main
| type: directory | ||
| uid: 10000 | ||
| gid: 10000 | ||
| permissions: 0o777 |
There was a problem hiding this comment.
Fix permissions syntax and reduce overly permissive settings.
Lines 31 and 36 use Python octal notation (0o777) which YAML parsers may not recognize. Additionally, 777 permissions (read/write/execute for everyone) violates the principle of least privilege. Use 0755 (or decimal 493) for both directories to allow the nonroot user full access while restricting write permissions for others.
- path: /home/nonroot
type: directory
uid: 10000
gid: 10000
- permissions: 0o777
+ permissions: 0755
- path: /usr/share/vocalizr
type: directory
uid: 10000
gid: 10000
- permissions: 0o777
+ permissions: 0755Also applies to: 36-36
🤖 Prompt for AI Agents
In .apko.yaml around lines 31 and 36, replace the Python-style octal literal
`0o777` with a YAML-friendly, least-privilege permission such as `0755` (as a
quoted string) or the decimal equivalent `493`; update both occurrences so
directories grant owner full access and restrict write for group/others instead
of world-writable.
| @@ -0,0 +1,46 @@ | |||
| package: | |||
| name: vocalizr | |||
| version: 0.0.13 | |||
There was a problem hiding this comment.
🧩 Analysis chain
Verify version consistency with pyproject.toml.
The package version is declared as 0.0.13 here, but past reviews flagged a mismatch with pyproject.toml (0.1.0). Confirm that versions are synchronized. If they differ intentionally, add a comment explaining the divergence.
Run the following script to verify the current version in pyproject.toml:
🏁 Script executed:
#!/bin/bash
# Check version in pyproject.toml
if [ -f "pyproject.toml" ]; then
grep "^version" pyproject.toml || grep 'version = ' pyproject.toml | head -1
else
echo "pyproject.toml not found"
fiLength of output: 17
Synchronize package versions or document intentional divergence.
Version mismatch confirmed: melange.yaml declares 0.0.13, while pyproject.toml declares 0.1.0. Either update both files to the same version or add an explanatory comment in each file clarifying why the versions differ intentionally.
🤖 Prompt for AI Agents
In melange.yaml around line 3, the declared version 0.0.13 conflicts with
pyproject.toml's 0.1.0; either synchronize them or document intentional
divergence: update melange.yaml to 0.1.0 (or pyproject.toml to 0.0.13) and
commit both files together, or add a clear comment in each file indicating why
versions intentionally differ and reference the authoritative source (e.g.,
release notes or packaging/version bump process); ensure the change is reflected
in any release/changelog automation.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uses: chainguard-images/actions/apko-publish@v1.0.7 | ||
| id: apko | ||
| with: | ||
| tag: ${{ steps.meta.outputs.tags }} |
There was a problem hiding this comment.
The apko-publish action is missing the required config parameter to specify the apko configuration file. Add config: .apko.yaml to the with: section to point to the apko configuration file introduced in this PR.
| tag: ${{ steps.meta.outputs.tags }} | |
| tag: ${{ steps.meta.outputs.tags }} | |
| config: .apko.yaml |
| echo "Check failed :x:" >> $GITHUB_STEP_SUMMARY | ||
| uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5 | ||
| - name: Install Melange | ||
| uses: addnab/docker-run-action@v3 |
There was a problem hiding this comment.
The action reference addnab/docker-run-action@v3 should use a commit hash instead of a mutable tag for better security and reproducibility, consistent with other actions in this workflow (e.g., line 31 uses commit hashes). Consider pinning to a specific commit SHA.
| build_apk: | ||
| name: Build APK with Melange | ||
| runs-on: ubuntu-latest | ||
| if: ${{ inputs.is_test }} |
There was a problem hiding this comment.
The build_apk job only runs when is_test is true, but build_image job depends on it and runs with if: ${{ always() && !cancelled() }}. This means that in non-test mode (production), the build_apk job will be skipped, but build_image will still try to run and fail because it won't have the required APK and melange key artifacts. Consider removing the condition on line 26 or adjusting the build_image job's condition to handle the case when build_apk is skipped.
| uses: mikefarah/yq@796317b885ae219215caa36e9bdacc87c9962c15 # v4.48.2 | ||
| uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5 | ||
| - name: Download APK artifact | ||
| uses: actions/download-artifact@v6.0.0 |
There was a problem hiding this comment.
The action reference actions/download-artifact@v6.0.0 should use a commit hash instead of a version tag for better security and reproducibility, consistent with other actions in this workflow (e.g., line 92 uses commit hashes). Consider pinning to a specific commit SHA.
| build-args: | | ||
| INSTALL_SOURCE=${{ inputs.install_source }} | ||
| PYTHON_VERSION=${{ steps.get_python_version.outputs.result }} | ||
| uses: chainguard-images/actions/apko-publish@v1.0.7 |
There was a problem hiding this comment.
The action reference chainguard-images/actions/apko-publish@v1.0.7 should use a commit hash instead of a version tag for better security and reproducibility, consistent with other actions in this workflow (e.g., line 92 uses commit hashes). Consider pinning to a specific commit SHA.
| uses: chainguard-images/actions/apko-publish@v1.0.7 | |
| uses: chainguard-images/actions/apko-publish@e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2e2 # v1.0.7 |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| permissions: 0o777 | ||
| - path: /usr/share/vocalizr | ||
| type: directory | ||
| uid: 10000 | ||
| gid: 10000 | ||
| permissions: 0o777 |
There was a problem hiding this comment.
Overly permissive directory permissions: Setting permissions to 0o777 (world-writable) is a security risk as it allows any user to modify these directories. Consider using 0o755 for /home/nonroot and 0o755 or 0o750 for /usr/share/vocalizr to maintain security while allowing the nonroot user necessary access.
| permissions: 0o777 | |
| - path: /usr/share/vocalizr | |
| type: directory | |
| uid: 10000 | |
| gid: 10000 | |
| permissions: 0o777 | |
| permissions: 0o755 | |
| - path: /usr/share/vocalizr | |
| type: directory | |
| uid: 10000 | |
| gid: 10000 | |
| permissions: 0o755 |
| - name: Sleep for 10 seconds | ||
| run: sleep 10s | ||
| - name: Checkout repository | ||
| uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5 |
There was a problem hiding this comment.
Inconsistent action version: This uses actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd while the same action at lines 35 and 92 uses actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8. For consistency and maintainability, all instances of the same action in a workflow should use the same version.
| uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5 | |
| uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5 |
| - api_test | ||
| - docker_scout | ||
| if: ${{ always() && !cancelled() && inputs.registry == 'ghcr.io' }} | ||
| if: ${{ inputs.registry == 'ghcr.io' }} |
There was a problem hiding this comment.
Changed conditional logic may cause issues: The condition was changed from ${{ always() && !cancelled() && inputs.registry == 'ghcr.io' }} to just ${{ inputs.registry == 'ghcr.io' }}. This means the cleanup job will no longer run if previous jobs fail or are cancelled. This could be intentional, but if the goal is to clean up old images regardless of build status, the always() && !cancelled() conditions should be retained.
| if: ${{ inputs.registry == 'ghcr.io' }} | |
| if: ${{ always() && !cancelled() && inputs.registry == 'ghcr.io' }} |
Remove __pycache__ directories after installing the package.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| run: melange keygen | ||
| shell: bash | ||
| - name: Build APK | ||
| uses: addnab/docker-run-action@v3 |
There was a problem hiding this comment.
Action reference should use a commit SHA for security, consistent with other actions in this workflow. Consider pinning to a specific commit instead of using @v3.
| with: | ||
| tag: ${{ steps.meta.outputs.tags }} | ||
| generic-user: mh0386 | ||
| generic-pass: ${{inputs.registry == 'ghcr.io' && secrets.GH_TOKEN || inputs.registry == 'docker.io' && secrets.TOKEN_KEY_DOCKER}} |
There was a problem hiding this comment.
The apko-publish action is missing the required config parameter to specify the apko configuration file path. This should be set to .apko.yaml to use the configuration file added in this PR.
| generic-pass: ${{inputs.registry == 'ghcr.io' && secrets.GH_TOKEN || inputs.registry == 'docker.io' && secrets.TOKEN_KEY_DOCKER}} | |
| generic-pass: ${{inputs.registry == 'ghcr.io' && secrets.GH_TOKEN || inputs.registry == 'docker.io' && secrets.TOKEN_KEY_DOCKER}} | |
| config: .apko.yaml |
| echo "Check failed :x:" >> $GITHUB_STEP_SUMMARY | ||
| uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5 | ||
| - name: Install Melange | ||
| uses: addnab/docker-run-action@v3 |
There was a problem hiding this comment.
Action reference should use a commit SHA for security, consistent with other actions in this workflow. Consider pinning to a specific commit instead of using @v3.
| type: directory | ||
| uid: 10000 | ||
| gid: 10000 | ||
| permissions: 0o777 |
There was a problem hiding this comment.
Setting permissions to 0o777 (world-writable) on /usr/share/vocalizr poses a security risk. Consider using more restrictive permissions like 0o755 to prevent unauthorized modifications.
| permissions: 0o777 | |
| permissions: 0o755 |
| name: apk | ||
| path: work/packages/ | ||
| - name: Download Melange key artifact | ||
| uses: actions/download-artifact@v6.0.0 |
There was a problem hiding this comment.
Action reference should use a commit SHA for security, consistent with other actions in this workflow. Consider pinning to a specific commit instead of using @v6.0.0.
| type: directory | ||
| uid: 10000 | ||
| gid: 10000 | ||
| permissions: 0o777 |
There was a problem hiding this comment.
Setting permissions to 0o777 (world-writable) on /home/nonroot poses a security risk as it allows any user to modify the home directory contents. Consider using more restrictive permissions like 0o755 or 0o700.
| permissions: 0o777 | |
| permissions: 0o700 |
| build-args: | | ||
| INSTALL_SOURCE=${{ inputs.install_source }} | ||
| PYTHON_VERSION=${{ steps.get_python_version.outputs.result }} | ||
| uses: chainguard-images/actions/apko-publish@v1.0.7 |
There was a problem hiding this comment.
Action reference should use a commit SHA for security, consistent with other actions in this workflow. Consider pinning to a specific commit instead of using @v1.0.7.
| uses: chainguard-images/actions/apko-publish@v1.0.7 | |
| uses: chainguard-images/actions/apko-publish@c7e2e2d1e4b2e2e2e6e7e2e2e2e2e2e2e2e2e2e2 # v1.0.7 |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.github/workflows/.docker.yaml (1)
113-118: Add requiredconfigparameter to apko-publish action.The
apko-publishaction requires aconfigparameter to specify the configuration file. Without this, the action cannot determine which apko configuration to use.- name: Build and Push to ${{ inputs.registry }} uses: chainguard-images/actions/apko-publish@v1.0.7 id: apko with: + config: .apko.yaml tag: ${{ steps.meta.outputs.tags }} generic-user: mh0386 generic-pass: ${{inputs.registry == 'ghcr.io' && secrets.GH_TOKEN || inputs.registry == 'docker.io' && secrets.TOKEN_KEY_DOCKER}}
🧹 Nitpick comments (4)
.github/workflows/.docker.yaml (4)
40-40: Pin melange image to specific version for reproducibility.Using
chainguard/melange:latest-dev(a floating tag) introduces unpredictability and potential breaking changes between builds. Consider pinning to a specific release version (e.g.,chainguard/melange:0.12.1) for reproducible builds.Also applies to: 48-48
37-37: Use commit SHA pinning for all actions for consistency and security.The workflow inconsistently pins actions: some use commit SHAs (e.g., lines 35, 92) while newer steps use version tags (lines 37, 45, 53, 58, 94, 113). Consider pinning all actions to specific commit SHAs for consistency and to improve security reproducibility. For example:
- name: Install Melange - uses: addnab/docker-run-action@v3 + uses: addnab/docker-run-action@<COMMIT_SHA> # v3This applies to:
addnab/docker-run-action@v3(lines 37, 45)actions/upload-artifact@v5(lines 53, 58)actions/download-artifact@v6.0.0(line 94)chainguard-images/actions/apko-publish@v1.0.7(line 113)Also applies to: 45-45, 53-53, 58-58, 94-94, 113-113
194-194: Use consistent checkout action version across workflow.Line 194 uses a different commit SHA (
93cb6efe...) foractions/checkout@v5than lines 35 and 92 (08c6903c...). For consistency, use the same version throughout the workflow.- name: Checkout repository - uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5 + uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5
216-216: Verify cleanup job intent: cleanup on all runs vs. success-only.The cleanup job condition was changed to only run when
inputs.registry == 'ghcr.io'(removing thealways() && !cancelled()condition). This means cleanup now only runs on successful builds. If the goal is to always cleanup old GHCR images (even on failed builds), restore thealways() && !cancelled()condition:- if: ${{ inputs.registry == 'ghcr.io' }} + if: ${{ always() && !cancelled() && inputs.registry == 'ghcr.io' }}Otherwise, this change is fine—it cleans up GHCR only on successful builds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/.docker.yaml(7 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). (4)
- GitHub Check: Test Image / Build APK with Melange
- GitHub Check: Lint / Format
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
| build_apk: | ||
| name: Build APK with Melange | ||
| runs-on: ubuntu-latest | ||
| if: ${{ inputs.is_test }} |
There was a problem hiding this comment.
Resolve artifact availability mismatch between build_apk and build_image jobs.
The build_apk job only runs when inputs.is_test is true (line 26), but build_image unconditionally runs (line 65) and attempts to download APK artifacts (lines 94–101). This causes the workflow to fail in non-test mode because the artifacts are never created.
Choose one of the following approaches:
Option A: Make build_apk always run (recommended if APK is always needed)
build_apk:
name: Build APK with Melange
runs-on: ubuntu-latest
- if: ${{ inputs.is_test }}
environment:
name: code_qualityOption B: Add if-no-files-found: ignore to artifact downloads (if APK is optional)
- name: Download APK artifact
uses: actions/download-artifact@v6.0.0
+ if: ${{ inputs.is_test }}
with:
name: apk
path: work/packages/
- name: Download Melange key artifact
uses: actions/download-artifact@v6.0.0
+ if: ${{ inputs.is_test }}
with:
name: melangeOption C: Make build_image conditional on is_test (if image only needed for testing)
build_image:
name: Build and push Docker image to ${{ inputs.registry }}
needs: build_apk
- if: ${{ always() && !cancelled() }}
+ if: ${{ inputs.is_test && always() && !cancelled() }}
runs-on: ubuntu-latestAlso applies to: 64-65, 94-101
🤖 Prompt for AI Agents
.github/workflows/.docker.yaml lines 26, 64-65, 94-101: the workflow currently
gates the build_apk job on inputs.is_test (line 26) but build_image runs
unconditionally (lines 64-65) and later tries to download APK artifacts (lines
94-101), causing failures when APK artifacts don't exist; fix by choosing one of
the three options: Option A (recommended) — remove or change the if on the
build_apk job so it always runs (delete the "if: ${{ inputs.is_test }}" at line
26 or set it to true) so the APK artifact is always produced; Option B — keep
current job gating but add if-no-files-found: ignore to the artifact download
steps at lines 94-101 so missing artifacts don’t fail the job; Option C — make
the build_image job conditional by adding the same "if: ${{ inputs.is_test }}"
at lines 64-65 so it only runs when APKs exist; apply the chosen change
consistently for both artifact downloads and job conditionals referenced above.
| with: | ||
| registry: cgr.dev | ||
| image: chainguard/melange:latest-dev | ||
| options: --rm -v ${{ github.workspace }}:/work |
There was a problem hiding this comment.
Add working directory flags to docker-run-action options.
The Melange commands (melange keygen and melange build) need to execute in /work where the workspace is mounted, but the current options don't set the working directory. Without -w /work, commands will run in the container's root and fail to find/generate configuration files and keys.
- name: Install Melange
uses: addnab/docker-run-action@v3
with:
registry: cgr.dev
image: chainguard/melange:latest-dev
- options: --rm -v ${{ github.workspace }}:/work
+ options: --rm -w /work -v ${{ github.workspace }}:/work
run: melange keygen
shell: bash
- name: Build APK
uses: addnab/docker-run-action@v3
with:
registry: cgr.dev
image: chainguard/melange:latest-dev
- options: --privileged --rm -v ${{ github.workspace }}:/work
+ options: --privileged --rm -w /work -v ${{ github.workspace }}:/work
run: melange build melange.yaml --arch amd64 --signing-key melange.rsa
shell: bashAlso applies to: 49-49
🤖 Prompt for AI Agents
.github/workflows/.docker.yaml around lines 41 and 49: the docker-run-action
options mount the workspace but don't set the container working directory, so
Melange commands run in the container root and can't find/generate files; update
the options values to include the working-dir flag (add -w /work) so the
container's process runs in the mounted workspace directory for both
occurrences.
| uses: actions/upload-artifact@v5 | ||
| with: | ||
| name: apk | ||
| path: packages/ |
There was a problem hiding this comment.
Fix artifact path mismatch between upload and download.
APK artifacts are uploaded from packages/ (line 56) but downloaded to work/packages/ (line 97). Update the upload path to match:
- name: Upload APK artifact
uses: actions/upload-artifact@v5
with:
name: apk
- path: packages/
+ path: work/packages/Also applies to: 97-97
🤖 Prompt for AI Agents
.github/workflows/.docker.yaml around line 56 (and also line 97): the artifact
upload path currently uses "packages/" but the workflow downloads from
"work/packages/" at line 97; update the upload step to use the same path by
changing the upload path to "work/packages/" so upload and download paths match
(ensure both occurrences reference the identical "work/packages/" directory).
|
|
Hi @MH0386, Your PR is in conflict and cannot be merged. |
Summary by Sourcery
Introduce apko and melange configuration files to automate building and packaging the vocalizr service in containers and OS packages.
New Features: