fix docker image notifications - #1245
Conversation
|
|
WalkthroughWorkflow now runs on pull_request events, captures short SHA and build start time, posts an initial "build started" PR comment, builds/pushes images, and always posts/updates a final PR comment (success with image details or failure with logs) using PR-based comment endpoints and concurrency cancellation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer (PR)
participant GH_Actions as GitHub Actions
participant GH_API as GitHub REST API (pull_request comments)
participant Registry as Image Registry
rect rgb(230,240,255)
Dev->>GH_Actions: open/update PR (pull_request)
GH_Actions->>GH_Actions: concurrency check\ncapture short SHA & start time
end
rect rgb(220,255,230)
GH_Actions->>GH_API: create/update "build started" PR comment (PR comment endpoints)
GH_API-->>GH_Actions: comment id / ack
end
rect rgb(255,245,220)
GH_Actions->>Registry: build & push image
Registry-->>GH_Actions: image tag/URL or failure
end
rect rgb(245,230,255)
GH_Actions->>GH_Actions: compute duration\nformat final message (success|failure)
GH_Actions->>GH_API: update/create final PR comment (PR comment endpoints)
GH_API-->>GH_Actions: ack
GH_Actions->>Dev: final comment visible on PR
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks✅ Passed checks (3 passed)
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:cdd9ba0
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/holmes:cdd9ba0 me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:cdd9ba0
docker push me-west1-docker.pkg.dev/robusta-development/development/holmes-dev:cdd9ba0Patch 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:cdd9ba0Robusta 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:cdd9ba0 |
af54d30 to
ba320f9
Compare
![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)
199-224: Comment update logic is correct but duplicated.This comment-finding and update/create logic is identical to lines 45-92. While functional, you could consider extracting this into a composite action or reusable workflow step if this pattern is used elsewhere.
16-19: Consider reducingcontentspermission toread.The workflow checks out the repository and builds a Docker image but doesn't appear to write to repository contents. You could potentially reduce
contents: 'write'tocontents: 'read'for better security posture (principle of least privilege).🔎 Proposed change
permissions: - contents: 'write' + contents: 'read' id-token: 'write' pull-requests: 'write'
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between af54d30cefeb88e343be8cc840087ac068eeff04 and eb6fdbc931aaa6dcca4c5bcd4ce5a8cbd4b1c52a.
📒 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
- GitHub Check: llm_evals
- GitHub Check: build (3.11)
- GitHub Check: build (3.12)
- GitHub Check: build (3.10)
🔇 Additional comments (5)
.github/workflows/docker-dev-images.yaml (5)
1-10: LGTM!The workflow trigger and concurrency control are well-configured. Using
github.head_reffor the concurrency group ensures that only one build runs per PR branch at a time, canceling stale runs on new pushes.
24-30: LGTM!The short SHA and start time capture steps are correctly implemented using
$GITHUB_OUTPUT.
32-92: Well-structured PR comment logic.The approach of using a marker comment (
<!-- docker-build-comment -->) to track and update a single comment is clean. The pagination for listing comments is appropriate, and preserving the previous image tag for reference is a nice UX touch.One consideration: if
context.payload.pull_requestis undefined (edge case), line 43 would throw. However, since the trigger is strictlypull_request, this should be safe.
140-146: Duration calculation relies on template string parsing.The duration calculation is functional. Minor note:
parseInton the template string works, but ifsteps.start_time.outputs.startis empty (e.g., if the step was skipped),parseIntreturnsNaN, causing incorrect duration display. This is unlikely given the workflow structure but worth noting.
168-169: Verify target registry region.The copy commands push to
me-west1-docker.pkg.devwhile the build pushes tous-central1-docker.pkg.dev. Ifme-west1(Middle East - Tel Aviv) is the intended permanent registry location, this is fine. Just confirming this isn't a typo for a different region.
cb53bdf to
59db1c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/docker-dev-images.yaml (3)
7-10: Consider using PR number for concurrency group.While
github.head_refworks for most cases, usinggithub.event.pull_request.numberis more idiomatic forpull_requestevents and avoids potential edge cases where multiple PRs might share the same head branch name.🔎 Proposed refinement
-# Cancel in-progress runs for the same PR concurrency: - group: docker-build-${{ github.head_ref }} + group: docker-build-pr-${{ github.event.pull_request.number }} cancel-in-progress: true
140-146: Consider adding validation for start_time output.The duration calculation assumes
steps.start_time.outputs.startis always available. While the start_time step should always execute, adding a fallback would make the workflow more robust.🔎 Proposed enhancement
- const startTime = parseInt(`${{ steps.start_time.outputs.start }}`); + const startTimeStr = `${{ steps.start_time.outputs.start }}`; + const startTime = startTimeStr ? parseInt(startTimeStr) : Math.floor(Date.now() / 1000); const endTime = Math.floor(Date.now() / 1000); const durationSecs = endTime - startTime;This provides a graceful fallback (duration ≈ 0) if start_time is unavailable.
148-197: Consider extracting registry paths as constants.The registry URLs are hardcoded in multiple places throughout the success message. Extracting them as JavaScript constants at the top of the script would improve maintainability.
🔎 Proposed refactor
At the beginning of the script block (after line 138), add:
const TEMP_REGISTRY = 'us-central1-docker.pkg.dev/robusta-development/temporary-builds'; const PERM_REGISTRY = 'me-west1-docker.pkg.dev/robusta-development/development'; const REGISTRY_LINK = 'https://console.cloud.google.com/artifacts/docker/robusta-development/us-central1/temporary-builds/holmes?project=robusta-development';Then update the message to use these constants:
const shortTag = `${TEMP_REGISTRY}/holmes:${shortSha}`; // ... and so on for other references
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between eb6fdbc931aaa6dcca4c5bcd4ce5a8cbd4b1c52a and 59db1c108cd0c73a6f2fce3dbf84c4d6a4602be7.
📒 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: build (3.11)
- GitHub Check: build (3.10)
- GitHub Check: llm_evals
- GitHub Check: build (3.12)
🔇 Additional comments (4)
.github/workflows/docker-dev-images.yaml (4)
16-19: LGTM! Good security practice.Downgrading
contentstoreadfollows the principle of least privilege while retaining necessary write permissions for PR comments and GCP authentication.
24-30: LGTM! Clean setup for downstream steps.The short SHA and start time capture are implemented correctly and will be used effectively in the comment steps.
32-93: LGTM! Well-implemented PR comment logic.The initial comment step correctly:
- Uses the issues API for PR comments
- Implements idempotent updates via marker pattern
- Preserves previous image information for user convenience
- Handles both creating new and updating existing comments
199-224: LGTM! Consistent PR comment implementation.The final comment logic correctly mirrors the initial comment step, maintaining consistency in how PR comments are created and updated throughout the workflow.
Add pull_request trigger so we get direct access to PR number via context.payload.pull_request. This ensures notifications link to the PR comment page rather than the commit comment page. Also adds concurrency control to prevent duplicate runs when both push and pull_request events fire for the same change. Signed-off-by: Robusta Runner <aantny@gmail.com>
Simplify workflow to only trigger on pull_request events. This ensures notifications always go to the PR and removes the complex fallback logic for commit comments. Signed-off-by: Robusta Runner <aantny@gmail.com>
- Post "Building..." comment when build starts with link to logs - Update same comment when build completes with success/failure status - Show build duration (e.g., "built in 5m 32s") - Collapse copy commands in a <details> section for cleaner UX - Handle build failures with error message and link to logs Signed-off-by: Robusta Runner <aantny@gmail.com>
When a new commit triggers a rebuild, preserve the previous image tag in the comment so users can still copy it while the new build runs. Signed-off-by: Robusta Runner <aantny@gmail.com>
Signed-off-by: Robusta Runner <aantny@gmail.com>
Signed-off-by: Robusta Runner <aantny@gmail.com>
029c3f2 to
2e1d361
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/docker-dev-images.yaml (1)
32-92: LGTM! Correct PR-based comment implementation.The step properly uses PR context and the issues API to post/update comments on the pull request itself, which fixes the notification issue described in the PR objectives. The marker-based approach prevents duplicate comments, and the previous image extraction is a nice user experience enhancement.
Consider extracting duplicated constants.
The marker string (
'<!-- docker-build-comment -->') and registry link are duplicated between this step and the final comment step (lines 138, 151). While not critical, extracting these to workflow-level environment variables would improve maintainability.🔎 Optional refactor to extract constants
Add environment variables at the workflow level:
+env: + DOCKER_COMMENT_MARKER: '<!-- docker-build-comment -->' + REGISTRY_LINK: 'https://console.cloud.google.com/artifacts/docker/robusta-development/us-central1/temporary-builds/holmes?project=robusta-development' + jobs: build: runs-on: ubuntu-latestThen reference them in the scripts:
- const marker = '<!-- docker-build-comment -->'; - const registryLink = 'https://console.cloud.google.com/artifacts/docker/robusta-development/us-central1/temporary-builds/holmes?project=robusta-development'; + const marker = process.env.DOCKER_COMMENT_MARKER; + const registryLink = process.env.REGISTRY_LINK;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 59db1c108cd0c73a6f2fce3dbf84c4d6a4602be7 and 2e1d361.
📒 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.12)
- GitHub Check: build (3.11)
- GitHub Check: build (3.10)
🔇 Additional comments (6)
.github/workflows/docker-dev-images.yaml (6)
1-10: LGTM! Trigger and concurrency changes align with PR objectives.The workflow now correctly triggers on pull request events and uses concurrency control to cancel in-progress builds for the same PR. The use of
github.head_refin the concurrency group is appropriate for pull_request events.
16-19: LGTM! Good security practice to reduce permissions.Changing
contentsfrom write to read follows the principle of least privilege, as this workflow only needs to read the repository and post PR comments.
24-26: LGTM! Standard approach for capturing short SHA.The step correctly uses
git rev-parse --short HEADand stores the output for use in subsequent steps.
28-30: LGTM! Simple and effective duration tracking.Recording the start time as a Unix timestamp enables accurate build duration calculation in the final comment step.
94-126: LGTM! Standard Docker build configuration.The Docker build steps are properly configured with multi-platform support and use the captured short SHA for tagging.
128-224: LGTM! Comprehensive final comment with proper failure handling.The
if: always()condition ensures notifications are posted even on build failure, which is essential for PR feedback. The step correctly captures${{ job.status }}(which provides "success", "failure", or "cancelled" values), calculates build duration, and branches to appropriate success or failure messages using the GitHub REST API.The success message includes helpful copy commands and Helm upgrade examples for developer testing.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/docker-dev-images.yaml (1)
197-198: Same registry URL verification needed.The same
me-west1-docker.pkg.devregistry prefix appears in the success message. See earlier comment about verifying this URL.Also applies to: 207-207, 214-215
🧹 Nitpick comments (1)
.github/workflows/docker-dev-images.yaml (1)
165-178: Consider checking the build step status instead of job status.Using
job.statuschecks the overall job status, which could be 'failure' even if the Docker build succeeded but a different step (e.g., checkout) failed. This would incorrectly show a build failure message.Consider checking the specific build step status for more accurate reporting.
🔎 Proposed refinement
Add an ID to the build step at line 139:
- name: Build and push Docker image + id: docker_build uses: docker/build-push-action@v6Then reference it in the comment step:
const shortSha = `${{ steps.short_sha.outputs.sha_short }}`; - const jobStatus = `${{ job.status }}`; + const buildSuccess = `${{ steps.docker_build.outcome }}` === 'success'; const runUrl = `https://github.com/${owner}/${repo}/actions/runs/${{ github.run_id }}`; let message; - if (jobStatus === 'success') { + if (buildSuccess) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 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: build (3.10)
- GitHub Check: llm_evals
- GitHub Check: build (3.12)
- GitHub Check: build (3.11)
🔇 Additional comments (3)
.github/workflows/docker-dev-images.yaml (3)
16-19: LGTM!The permission changes are appropriate. Reducing
contentsfromwritetoreadfollows the principle of least privilege, whilepull-requests: writeenables the PR commenting feature.
24-30: LGTM!The setup steps correctly capture the short SHA and build start time for use in comments and duration calculation.
32-122: Excellent approach to PR-based notifications.The workflow successfully achieves the PR objective by using
context.payload.pull_request.numberand PR-based comment endpoints (issues.listComments,issues.createComment,issues.updateComment) throughout. The marker-based comment finding ensures updates to the same comment across builds, and theif: always()condition guarantees status is reported even on failures.Also applies to: 157-253
Results of HolmesGPT evals
Legend
|
They're currently being attached to files and not to the PR itself <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Workflow now triggers on pull requests (opened/synchronize/reopened) and cancels in-progress runs for the same head branch. * Reduces repo contents permission from write to read while retaining identity and PR permissions. * Captures build start time and short commit SHA (duplicate retrieval removed). * Posts/updates a "build started" comment on the PR and consolidates all comment operations to PR-based endpoints. * Always posts a final PR comment with computed build duration and detailed success/failure messaging. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Robusta Runner <aantny@gmail.com> Signed-off-by: Filip Grebowski <grebowskifilip@gmail.com>
3 participants
They're currently being attached to files and not to the PR itself
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.