Repository navigation
Skip building images on PRs from external users (forks) - #1249
Conversation
|
|
WalkthroughDetects fork PRs in the Docker dev images GitHub Actions workflow and branches execution: fork PRs run a build-only path (no registry auth or push, limited platforms) and skip final PR comments; non-fork PRs perform authenticated multi-platform build-and-push and post final PR messages. Start time captured for duration reporting; old-style PR comments cleaned up at start. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant ForkCheck as fork_check
participant Buildx as Docker Buildx
participant Registry as Container Registry / gcloud
participant PR as PR comment service
GH->>ForkCheck: run fork_check -> set is_fork
alt is_fork == 'true' (fork PR)
GH->>Buildx: Build image (push: false, limited platforms)
Buildx-->>GH: build result
note right of GH `#f8f0e6`: Skip registry auth, gcloud setup, push\nSkip final PR comment
else is_fork != 'true' (non-fork PR)
GH->>Registry: Authenticate (gcloud / docker login)
Registry-->>GH: auth token
GH->>Buildx: Build & Push multi-platform image (linux/amd64, linux/arm64)
Buildx-->>Registry: push image & tags
Registry-->>GH: image URLs
GH->>PR: Post final image-ready comment (includes duration)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📥 CommitsReviewing files that changed from the base of the PR and between 93f5b743378b1e1a79540469662ebbdcea3b311e and 80e8496. 📒 Files selected for processing (1)
⏰ 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)
🔇 Additional comments (7)
Comment |
|
✅ Docker image ready for Use this tag to pull the image for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:3522276
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:3522276 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:3522276
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:3522276Patch Helm values in one line (choose the chart you use): HolmesGPT chart: helm upgrade --install holmesgpt ./helm/holmes \
--set registry=me-west1-docker.pkg.dev/robusta-development/development \
--set image=holmes-dev:3522276Robusta wrapper chart: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set holmes.registry=me-west1-docker.pkg.dev/robusta-development/development \
--set holmes.image=holmes-dev:3522276 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/docker-dev-images.yaml (2)
42-133: Consider adding minimal feedback for fork PR contributors.Fork PR contributors receive no comments about build status since this step is skipped. While the build-only approach is correct, contributors might benefit from a simple comment indicating the build succeeded (without registry details).
Optional: Add fork-specific build notification
You could add a separate, simpler comment step for fork PRs after the build completes:
- name: Comment build status (fork PR) if: always() && steps.fork_check.outputs.is_fork == 'true' uses: actions/github-script@v7 with: script: | const shortSha = `${{ steps.short_sha.outputs.sha_short }}`; const jobStatus = `${{ job.status }}`; const marker = '<!-- docker-build-fork-comment -->'; const message = jobStatus === 'success' ? `✅ Docker build succeeded for \`${shortSha}\` (validation only - not pushed to registry)` : `❌ Docker build failed for \`${shortSha}\``; github.rest.issues.createComment({ owner: context.repo.owner, repo: context.repo.repo, issue_number: context.payload.pull_request.number, body: marker + '\n' + message });
154-164: Fork PRs skip ARM64 platform validation.Fork PRs build only for
linux/amd64, while non-fork PRs build for bothlinux/arm64andlinux/amd64. This means fork contributors won't discover ARM compatibility issues until after their PR is merged or a maintainer reviews it.Consider whether ARM64 validation is important enough to include for fork PRs:
Option: Include both platforms for fork PRs
- name: Build Docker image (fork PR - no push) if: steps.fork_check.outputs.is_fork == 'true' uses: docker/build-push-action@v6 with: context: . - platforms: linux/amd64 + platforms: linux/arm64,linux/amd64 push: false build-args: | BUILDKIT_INLINE_CACHE=1This will increase build time for fork PRs but provides better validation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between bcfc05b and dcfb8dd023c62e0df8f326d27588f9cacd517e56.
📒 Files selected for processing (1)
.github/workflows/docker-dev-images.yaml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: HolmesGPT/holmesgpt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-25T11:30:26.525Z
Learning: PRs require maintainer approval and pre-commit hooks must pass before merging
⏰ 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: build (3.10)
- GitHub Check: llm_evals
- GitHub Check: build
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
🔇 Additional comments (4)
.github/workflows/docker-dev-images.yaml (4)
134-149: LGTM! Correct security-conscious gating.The authentication and registry configuration steps are properly gated to run only for non-fork PRs. This prevents fork PRs from attempting operations they don't have permissions for and aligns with GitHub's security model for fork PRs.
166-177: LGTM! Multi-platform build and push configured correctly.The non-fork PR build correctly targets both ARM64 and AMD64 platforms and pushes with appropriate tags. The conditional ensures this only runs when registry access is available.
186-282: LGTM! Comprehensive PR feedback for non-fork PRs.The final comment step correctly:
- Uses
always()to provide feedback even on failure- Gates execution for non-fork PRs only
- Calculates and displays build duration
- Provides detailed registry information and deployment commands
The conditional logic ensures fork PRs don't attempt operations requiring write permissions.
24-32: The fork detection logic is correct as-is. Thegithub.event.pull_request.head.repo.forkproperty is a boolean within thepull_requestevent context (which is this workflow's trigger), so it will never be null or undefined. The bash comparison[ "${{ github.event.pull_request.head.repo.fork }}" = "true" ]is the standard and appropriate way to check this property in GitHub Actions workflows.
dcfb8dd to
c57799c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/docker-dev-images.yaml (4)
24-32: Strengthen fork detection logic.The current check assumes
github.event.pull_request.head.repo.forkis either"true"or exists with a different value. For same-repo PRs, this property might benull,undefined, or missing entirely. While the else branch handles this implicitly, a more explicit comparison improves robustness.🔎 Suggested improvement
- name: Check if PR is from fork id: fork_check run: | - if [ "${{ github.event.pull_request.head.repo.fork }}" = "true" ]; then + if [ "${{ github.event.pull_request.head.repo.full_name }}" != "${{ github.repository }}" ]; then echo "is_fork=true" >> $GITHUB_OUTPUT echo "📌 PR is from a fork - will build but not push" else echo "is_fork=false" >> $GITHUB_OUTPUT fiThis compares the full repository names, which is more explicit and reliable than checking the
forkboolean property.
43-43: Consider UX for external contributors.Skipping all PR comments for fork contributors means they won't receive any feedback about build status. While they can view logs directly in the Actions tab, a simple comment indicating "Build validation running (images won't be published for fork PRs)" would improve contributor experience.
💡 Suggested enhancement
Consider adding a lightweight comment for fork PRs:
- name: Comment build started uses: actions/github-script@v7 with: script: | const owner = context.repo.owner; const repo = context.repo.repo; const shortSha = `${{ steps.short_sha.outputs.sha_short }}`; const runUrl = `https://github.com/${owner}/${repo}/actions/runs/${{ github.run_id }}`; const marker = '<!-- docker-build-comment -->'; const isFork = '${{ steps.fork_check.outputs.is_fork }}' === 'true'; let message; if (isFork) { message = [ marker, `🔨 **Building Docker image for \`${shortSha}\` (fork PR - validation only)...**`, '', `[View build logs](${runUrl})`, '', '📌 Images from fork PRs are not published to the registry.', ].join('\n'); } else { // ... existing non-fork message logic ... } // ... rest of comment posting logic ...
154-164: PR title doesn't match implementation.The PR title states "Skip building images on PRs from external users (forks)" but the implementation actually builds images for forks—it just doesn't push them. This validates that fork PRs don't break the Dockerfile, which is valuable for maintainers reviewing external contributions.
Consider updating the PR title to "Build but don't push images for fork PRs" or similar to accurately reflect the behavior.
Also note that fork builds only target
linux/amd64while non-fork builds are multi-platform. This is a reasonable optimization for validation speed, but consider whether both platforms should be validated to catch platform-specific build issues early.
186-282: Fork PRs receive no build status feedback in PR comments.Combined with skipping the "build started" comment (line 43), fork contributors receive no PR comments about build status—neither start, success, nor failure notifications. While they can view workflow logs in the Actions tab, this creates a less welcoming experience for external contributors.
Consider posting a lightweight success/failure comment for fork PRs, even if it doesn't include image push details.
💡 Example implementation
- name: Comment with build status (fork PRs) if: always() && steps.fork_check.outputs.is_fork == 'true' uses: actions/github-script@v7 with: script: | const shortSha = `${{ steps.short_sha.outputs.sha_short }}`; const jobStatus = `${{ job.status }}`; const runUrl = `https://github.com/${context.repo.owner}/${context.repo.repo}/actions/runs/${{ github.run_id }}`; const marker = '<!-- docker-build-comment -->'; const message = jobStatus === 'success' ? `${marker}\n✅ **Docker build validated for \`${shortSha}\`** (fork PR)\n\n📌 Images from fork PRs are not published.\n[View logs](${runUrl})` : `${marker}\n❌ **Docker build failed for \`${shortSha}\`**\n\n[View logs](${runUrl})`; // ... comment posting logic similar to lines 257-282 ...
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between dcfb8dd023c62e0df8f326d27588f9cacd517e56 and c57799c3d61f016a388911bc7fd5c91ddddeb7f1.
📒 Files selected for processing (1)
.github/workflows/docker-dev-images.yaml
⏰ 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: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build
- GitHub Check: llm_evals
🔇 Additional comments (2)
.github/workflows/docker-dev-images.yaml (2)
134-149: LGTM! Correct security boundary.The conditionals properly skip authentication and registry configuration for fork PRs. Fork workflows don't have access to the OIDC token required for Google Cloud authentication, so these steps would fail anyway. This approach prevents unnecessary errors.
166-178: LGTM! Non-fork path preserved correctly.The conditional ensures that non-fork PRs continue to build and push multi-platform images as before. The logic correctly maintains the existing behavior for internal contributors.
c57799c to
93f5b74
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/docker-dev-images.yaml (1)
43-43: Fork contributors receive no status comments.The conditional correctly skips the build-started comment for fork PRs. However, fork contributors will have no visibility into build progress except through the GitHub Actions tab. While this aligns with the PR objective, consider adding a simple informational comment for fork PRs (e.g., "Build started - images will not be pushed for fork PRs") to improve contributor experience.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between c57799c3d61f016a388911bc7fd5c91ddddeb7f1 and 93f5b743378b1e1a79540469662ebbdcea3b311e.
📒 Files selected for processing (1)
.github/workflows/docker-dev-images.yaml
⏰ 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: build
- GitHub Check: llm_evals
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
🔇 Additional comments (4)
.github/workflows/docker-dev-images.yaml (4)
142-157: LGTM: Auth steps correctly gated.The authentication, gcloud setup, and Docker registry configuration steps are all correctly gated to skip for fork PRs. This prevents unnecessary execution and potential confusing error messages, since fork PRs wouldn't have access to the required credentials due to GitHub's security model anyway.
162-172: LGTM: Fork PR build path correctly implemented.The build-only step for fork PRs is correctly configured with
push: falseand a single platform (linux/amd64). This provides validation of the Dockerfile changes without attempting to push to the registry, and the single-platform build is a reasonable optimization for external contributions.
174-186: LGTM: Non-fork build and push correctly configured.The multi-platform build and push step is properly gated to only run for non-fork PRs. The dual-platform build (
linux/arm64,linux/amd64) and dual-tag strategy (full SHA and short SHA) provide comprehensive testing capabilities for maintainer PRs.
187-195: LGTM: Final steps correctly gated.The image location print and final comment steps are correctly gated to skip for fork PRs. The use of
always()combined with the fork check on line 195 ensures that non-fork PRs receive status updates (success or failure) while fork PRs are consistently excluded from the commenting flow.
For PRs from forks, GitHub automatically downgrades GITHUB_TOKEN to read-only access for security. Changes: - Fork PRs: Build Docker image (no push) to verify the code compiles - Fork PRs: Skip gcloud auth, registry config, and PR comments - Non-fork PRs: Full build + push + PR comments (unchanged behavior) GitHub automatically protects secrets for fork PRs - they are not passed to workflows triggered by pull_request from forks.
93f5b74 to
80e8496
Compare
Results of HolmesGPT evals
Legend
|
4 participants
It wont succeed anyway
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.