feat: add pre-build-targets for running lint/test before final build #15
Conversation
📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant GHAction as GitHub Action (workflow)
participant Depot
User->>GHAction: Trigger workflow
activate GHAction
opt depot-token present
alt pre-build-targets provided
GHAction->>GHAction: parse `pre-build-targets` (comma-separated)
loop per target
GHAction->>Depot: depot build [target]
Depot-->>GHAction: result
end
else no pre-build-targets
GHAction->>GHAction: auto-detect targets from Dockerfile (lint/test)
opt auto-targets found
loop per auto-target
GHAction->>Depot: depot build [auto-target]
Depot-->>GHAction: result
end
end
end
end
GHAction->>Depot: depot build [main build & push]
Depot-->>GHAction: build/push result
GHAction-->>User: Workflow complete
deactivate GHAction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
action.yml(2 hunks)
🔇 Additional comments (2)
action.yml (2)
61-64: Input definition looks good.The new
pre-build-targetsinput follows the existing pattern—optional, well-described, and defaults to empty. This integrates cleanly with the existing action interface.
151-159: No changes needed — the--platformflag syntax is correct.The
depot buildcommand supports the--platformflag with comma-separated formats like--platform linux/amd64,linux/arm64. The code on line 154 correctly uses--platform ${{ inputs.platforms }}and is fully compatible with the Depot CLI.
| - name: Run pre-build targets with Depot | ||
| if: ${{ inputs.depot-token != '' && inputs.pre-build-targets != '' }} | ||
| shell: bash | ||
| run: | | ||
| IFS=',' read -ra TARGETS <<< "${{ inputs.pre-build-targets }}" | ||
| for target in "${TARGETS[@]}"; do | ||
| echo "Building target: $target" | ||
| depot build \ | ||
| --project ${{ inputs.depot-project }} \ | ||
| --token ${{ inputs.depot-token }} \ | ||
| --platform ${{ inputs.platforms }} \ | ||
| --target "$target" \ | ||
| --secret id=pipconf,src=/tmp/build-secrets/pipconf \ | ||
| --secret id=uvconfig,src=/tmp/build-secrets/uvconfig.toml \ | ||
| --file ${{ inputs.dockerfile }} \ | ||
| ${{ inputs.docker-build-context }} | ||
| done |
There was a problem hiding this comment.
Missing error handling and whitespace trimming in target parsing.
The pre-build step has two issues:
-
No error handling: If a
depot buildcommand fails for any target, the loop continues silently and the main build proceeds. This defeats the purpose of validation stages (lint, test, etc.). You likely want to fail the workflow if any pre-build target fails. -
Whitespace not trimmed: If users provide
"lint, test"(with spaces), the parsed target becomes" test"with a leading space, which won't match the actual target name"test".
Apply this diff to add error handling and trim whitespace:
- name: Run pre-build targets with Depot
if: ${{ inputs.depot-token != '' && inputs.pre-build-targets != '' }}
shell: bash
run: |
IFS=',' read -ra TARGETS <<< "${{ inputs.pre-build-targets }}"
for target in "${TARGETS[@]}"; do
+ target="${target#"${target%%[![:space:]]*}"}"
+ target="${target%"${target##*[![:space:]]}"}"
echo "Building target: $target"
depot build \
--project ${{ inputs.depot-project }} \
--token ${{ inputs.depot-token }} \
--platform ${{ inputs.platforms }} \
--target "$target" \
--secret id=pipconf,src=/tmp/build-secrets/pipconf \
--secret id=uvconfig,src=/tmp/build-secrets/uvconfig.toml \
--file ${{ inputs.dockerfile }} \
- ${{ inputs.docker-build-context }}
+ ${{ inputs.docker-build-context }} || exit 1
doneAlternatively, use a cleaner approach with xargs:
- name: Run pre-build targets with Depot
if: ${{ inputs.depot-token != '' && inputs.pre-build-targets != '' }}
shell: bash
run: |
echo "${{ inputs.pre-build-targets }}" | tr ',' '\n' | while read -r target; do
target="${target#"${target%%[![:space:]]*}"}"
target="${target%"${target##*[![:space:]]}"}"
[ -z "$target" ] && continue
echo "Building target: $target"
depot build \
--project ${{ inputs.depot-project }} \
--token ${{ inputs.depot-token }} \
--platform ${{ inputs.platforms }} \
--target "$target" \
--secret id=pipconf,src=/tmp/build-secrets/pipconf \
--secret id=uvconfig,src=/tmp/build-secrets/uvconfig.toml \
--file ${{ inputs.dockerfile }} \
${{ inputs.docker-build-context }} || exit 1
done📝 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.
| - name: Run pre-build targets with Depot | |
| if: ${{ inputs.depot-token != '' && inputs.pre-build-targets != '' }} | |
| shell: bash | |
| run: | | |
| IFS=',' read -ra TARGETS <<< "${{ inputs.pre-build-targets }}" | |
| for target in "${TARGETS[@]}"; do | |
| echo "Building target: $target" | |
| depot build \ | |
| --project ${{ inputs.depot-project }} \ | |
| --token ${{ inputs.depot-token }} \ | |
| --platform ${{ inputs.platforms }} \ | |
| --target "$target" \ | |
| --secret id=pipconf,src=/tmp/build-secrets/pipconf \ | |
| --secret id=uvconfig,src=/tmp/build-secrets/uvconfig.toml \ | |
| --file ${{ inputs.dockerfile }} \ | |
| ${{ inputs.docker-build-context }} | |
| done | |
| - name: Run pre-build targets with Depot | |
| if: ${{ inputs.depot-token != '' && inputs.pre-build-targets != '' }} | |
| shell: bash | |
| run: | | |
| IFS=',' read -ra TARGETS <<< "${{ inputs.pre-build-targets }}" | |
| for target in "${TARGETS[@]}"; do | |
| target="${target#"${target%%[![:space:]]*}"}" | |
| target="${target%"${target##*[![:space:]]}"}" | |
| echo "Building target: $target" | |
| depot build \ | |
| --project ${{ inputs.depot-project }} \ | |
| --token ${{ inputs.depot-token }} \ | |
| --platform ${{ inputs.platforms }} \ | |
| --target "$target" \ | |
| --secret id=pipconf,src=/tmp/build-secrets/pipconf \ | |
| --secret id=uvconfig,src=/tmp/build-secrets/uvconfig.toml \ | |
| --file ${{ inputs.dockerfile }} \ | |
| ${{ inputs.docker-build-context }} || exit 1 | |
| done |
🤖 Prompt for AI Agents
In action.yml around lines 144 to 160, the pre-build loop neither trims
whitespace from comma-separated targets nor fails the workflow when a depot
build fails; update the shell block to (1) make the script fail fast by enabling
strict mode (e.g. set -euo pipefail) or by testing each depot build's exit
status and exiting with that status on failure, and (2) trim whitespace from
each parsed target before use (e.g. trim with xargs or shell parameter
expansion) and ensure inputs are properly quoted when passed to depot so targets
like "lint, test" become "lint" and "test" and any failed depot build aborts the
workflow.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
action.yml (1)
175-191: Address critical issues from previous review: error handling and whitespace trimming.This review comment duplicates concerns from the previous code review. The pre-build loop has two unresolved critical issues:
No error handling: If any
depot buildfails, the loop continues silently and the workflow proceeds to the main build step (line 192). This defeats the purpose of validation stages. The workflow should fail immediately.Whitespace not trimmed: If users provide targets like
"lint, test"(with spaces), parsing produces["lint", " test"]. The leading space in" test"won't match the actual target name"test", causing silent failures.Apply this diff to fix both issues:
- name: Run pre-build targets with Depot if: ${{ inputs.depot-token != '' && steps.detect-stages.outputs.auto-targets != '' }} shell: bash run: | - IFS=',' read -ra TARGETS <<< "${{ steps.detect-stages.outputs.auto-targets }}" + set -euo pipefail + echo "${{ steps.detect-stages.outputs.auto-targets }}" | tr ',' '\n' | while read -r target; do + target=$(echo "$target" | xargs) # Trim leading/trailing whitespace + [ -z "$target" ] && continue - for target in "${TARGETS[@]}"; do echo "Building target: $target" depot build \ --project ${{ inputs.depot-project }} \ --token ${{ inputs.depot-token }} \ --platform ${{ inputs.platforms }} \ --target "$target" \ --secret id=pipconf,src=/tmp/build-secrets/pipconf \ --secret id=uvconfig,src=/tmp/build-secrets/uvconfig.toml \ --file ${{ inputs.dockerfile }} \ - ${{ inputs.docker-build-context }} + ${{ inputs.docker-build-context }} || exit 1 doneKey changes:
set -euo pipefailenables strict mode to fail on errorstr ',' '\n'splits by comma, thenxargstrims whitespace from each target[ -z "$target" ] && continueskips empty targets|| exit 1ensures the workflow fails if any pre-build target fails
🧹 Nitpick comments (1)
action.yml (1)
144-174: Consider making stage detection regex more robust.The grep patterns (
^FROM .* AS lint) are restrictive and won't match Dockerfile variations like extra spaces (FROM ubuntu AS lint). While the Dockerfile spec requiresFROMat the line start, a more flexible pattern would be safer:grep -qi "^FROM .* AS lint" "$DOCKERFILE" # Add -i for case-insensitive matchingThis is optional if your Dockerfiles follow strict formatting conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
action.yml(2 hunks)
🔇 Additional comments (1)
action.yml (1)
61-64: Input definition looks good.Clear description and sensible defaults.
Summary by CodeRabbit