Conversation
|
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. Warning Rate limit exceeded@ejfine has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
WalkthroughAdds vitest_version to template context in two places, introduces an artifact-name output in the reusable Docker build workflow, and removes the devcontainer manual dependency setup script template. No control-flow changes to existing hooks or jobs. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR exposes the artifact name from the build-docker-image workflow as an output and adds vitest version context for template generation. The primary purpose is to make the Docker image artifact name accessible to downstream pipeline steps.
- Adds workflow output for the Docker image artifact name
- Includes vitest version in context for template rendering
- Removes an unused manual setup script
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| template/.github/reusable_workflows/build-docker-image.yaml.jinja-base | Adds workflow and job outputs to expose artifact name |
| template/extensions/context.py.jinja-base | Adds vitest_version to template context |
| extensions/context.py | Sets vitest version to ^3.2.4 in context |
| template/.devcontainer/manual-setup-deps.sh.jinja-base | Removes entire manual setup dependencies script |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| name: Build Docker Image | ||
| runs-on: {% endraw %}{{ gha_linux_runner }}{% raw %} | ||
| outputs: | ||
| artifact-name: ${{ steps.calculate-build-context-hash.outputs.image_name_no_slashes }} |
There was a problem hiding this comment.
The output references steps.calculate-build-context-hash.outputs.image_name_no_slashes but there's no visible step with id calculate-build-context-hash in the diff. Verify this step exists and produces the expected output.
![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
🧹 Nitpick comments (5)
extensions/context.py (1)
53-53: Keep source-of-truths in syncSince this repo sets versions both here and in the template Jinja base, consider adding a brief comment line next to vitest_version noting it must remain in sync with template/extensions/context.py.jinja-base (prevents drift).
template/.github/reusable_workflows/build-docker-image.yaml.jinja-base (4)
47-49: Propagate additional outputs to improve downstream UX (optional)Downstream steps often need more than the artifact “name”:
- The artifact filename (with .tar) for docker load or checksum.
- The build-context tag (already computed as build_context_tag) to disambiguate artifacts and images.
Suggest exporting both at the job level and surfacing them at workflow_call. This is backward-compatible and makes consumers simpler.
Apply this diff to the changed block to add job outputs:
runs-on: {{ gha_linux_runner }} outputs: artifact-name: ${{ steps.calculate-build-context-hash.outputs.image_name_no_slashes }} + artifact-filename: ${{ steps.calculate-build-context-hash.outputs.artifact_filename }} + build-context-tag: ${{ steps.calculate-build-context-hash.outputs.build_context_tag }}And extend the workflow_call outputs:
outputs: artifact-name: description: 'The name of the uploaded artifact of the image tarball' value: ${{ jobs.build-image.outputs.artifact-name }} + artifact-filename: + description: 'The filename of the uploaded tarball (without path)' + value: ${{ jobs.build-image.outputs.artifact-filename }} + build-context-tag: + description: 'The calculated tag derived from the build context hash' + value: ${{ jobs.build-image.outputs.build-context-tag }}Outside the changed ranges, add one line to the “Calculate hash of files in build context” step to emit artifact_filename:
# inside the run: block after setting IMAGE_NAME_NO_SLASHES echo "artifact_filename=${IMAGE_NAME_NO_SLASHES}.tar" >> "$GITHUB_OUTPUT"
47-49: Consider clarifying semantics when save-as-artifact is falseThe artifact-name output is always set even if no artifact is uploaded. This is fine, but it can confuse callers. Two low-effort options:
- Document that artifact-name is meaningful only when inputs.save-as-artifact == true.
- Or gate the job output behind a conditional step that only emits when saving.
Happy to add a short README snippet or usage example.
154-160: Optional: include build-context tag in the artifact name to avoid collisionsIf this reusable workflow is invoked multiple times within the same run building the same image_name but different build contexts, artifact names can collide and merge. Including the build_context_tag in the artifact name avoids that.
Proposed change outside the current diff:
- name: ${{ steps.calculate-build-context-hash.outputs.image_name_no_slashes }} + name: ${{ steps.calculate-build-context-hash.outputs.image_name_no_slashes }}--${{ steps.calculate-build-context-hash.outputs.build_context_tag }}If you prefer to keep the shorter name, exposing build-context-tag (suggested above) still lets consumers disambiguate.
77-90: Minor: surface artifact info in the job summary for easier debuggingNot required, but adding a short step to append the artifact name and build-context tag to $GITHUB_STEP_SUMMARY helps troubleshoot downloads in caller workflows.
Example snippet to add after the hash step:
- name: Summarize artifact info if: ${{ always() }} run: | { echo "### Build image artifact"; echo "- image_name_no_slashes: ${{ steps.calculate-build-context-hash.outputs.image_name_no_slashes }}"; echo "- build_context_tag: ${{ steps.calculate-build-context-hash.outputs.build_context_tag }}"; echo "- will_upload_artifact: ${{ inputs.save-as-artifact }}"; } >> "$GITHUB_STEP_SUMMARY"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
extensions/context.py(1 hunks)template/.devcontainer/manual-setup-deps.sh.jinja-base(0 hunks)template/.github/reusable_workflows/build-docker-image.yaml.jinja-base(2 hunks)template/extensions/context.py.jinja-base(1 hunks)
💤 Files with no reviewable changes (1)
- template/.devcontainer/manual-setup-deps.sh.jinja-base
🔇 Additional comments (4)
template/extensions/context.py.jinja-base (1)
48-48: Good addition: vitest_version wired into template contextThis mirrors the surrounding pattern and keeps the JS tooling versions consistent in the context hook.
extensions/context.py (1)
53-53: LGTM: introduces vitest_version with sane semver rangePlacement and naming match the existing Node tool versions block. No behavioral risk.
template/.github/reusable_workflows/build-docker-image.yaml.jinja-base (2)
34-37: Output exposed correctly at workflow_callThe artifact-name output is plumbed through to callers via jobs.build-image.outputs.artifact-name. Syntax and scoping look correct.
34-37: Verify that the newartifact-nameoutput is actually consumedI’ve searched both
template/and.github/workflowsfor any callers of thebuild-docker-image.yaml.jinja-baseworkflow or references tooutputs.artifact-name/actions/download-artifact, but found none.• No internal workflows call
uses: .github/reusable_workflows/build-docker-image.yaml.jinja-base• No references to
${{ jobs.build-image.outputs.artifact-name }}• No uses of the
actions/download-artifactstepPlease manually verify that any external workflows (in consuming repositories) have been updated to pull the
artifact-nameoutput and pass it toactions/download-artifact.If you haven’t yet added any callers, consider including a minimal example workflow in
template/.github/workflows/that demonstrates downloading the image tarball via the exposedartifact-nameoutput.
| context["vue_version"] = "{{ vue_version }}" | ||
| context["vue_router_version"] = "{{ vue_router_version }}" | ||
| context["faker_version"] = "{{ faker_version }}" | ||
| context["vitest_version"] = "{{ vitest_version }}" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify vitest_version is defined in copier.yaml and test fixtures
To avoid Jinja UndefinedError during template rendering, ensure vitest_version is present wherever other version vars are defined (e.g., copier.yaml and tests/data*.yml).
Run this to confirm coverage:
If missing, I can add a default in copier.yaml matching extensions/context.py ("^3.2.4").
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Search for vitest_version definitions in config and tests"
# copier config(s)
fd -a -t f 'copier.y*ml' | xargs -r -I{} rg -n --no-heading '^\s*vitest_version\s*:' {} || true
# test fixtures
rg -n --no-heading 'vitest_version' tests || true
# ensure used in templates (expect hits)
rg -n --no-heading 'vitest_version' template || trueLength of output: 443
Define vitest_version in copier.yaml and test fixtures
The template at template/extensions/context.py.jinja-base:48 uses {{ vitest_version }}, but our search shows this variable isn’t defined in any copier configuration or in the test fixtures—this will trigger a Jinja UndefinedError when rendering.
Please add a default vitest_version in your copier config and mirror it in your test data. For example:
• In copier.yaml (or .copier.yaml), under your variables section:
variables:
# …existing definitions…
+ vitest_version:
+ type: str
+ default: "^3.2.4"
+ help: "Version of Vitest to install in the generated project"• In your test fixtures (e.g. tests/data-common.yml or similar):
# …existing test data…
+vitest_version: "^3.2.4"These additions will ensure that vitest_version is always defined during both normal usage and in CI/test runs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| context["vitest_version"] = "{{ vitest_version }}" | |
| # In copier.yaml (or .copier.yaml), under your `variables:` section | |
| variables: | |
| # …existing definitions… | |
| vitest_version: | |
| type: str | |
| default: "^3.2.4" | |
| help: "Version of Vitest to install in the generated project" |
| context["vitest_version"] = "{{ vitest_version }}" | |
| # In your test fixtures file (e.g. tests/data-common.yml) | |
| # …existing test data… | |
| vitest_version: "^3.2.4" |
🤖 Prompt for AI Agents
In template/extensions/context.py.jinja-base around line 48, the template
references {{ vitest_version }} but that variable is not defined in copier
configuration or test fixtures; add a default vitest_version entry to your
copier config (e.g., in copier.yaml / .copier.yaml under the variables section)
and add the same key/value to your test fixture data file (e.g.,
tests/data-common.yml or the fixtures file your test suite loads) so rendering
never raises a Jinja UndefinedError.
Why is this change necessary?
Needs to be accessible for other steps in a pipeline that download that artifact
How does this change address the issue?
Adds it as an output
What side effects does this change have?
None
How is this change tested?
Downstream repo
Other
Add vitest version in context
Summary by CodeRabbit