feat: add Homebrew formula and update workflow for automatic formula …#818
feat: add Homebrew formula and update workflow for automatic formula …#818stearz wants to merge 1 commit intokelos-dev:mainfrom
Conversation
There was a problem hiding this comment.
2 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="hack/update-homebrew-formula.sh">
<violation number="1" location="hack/update-homebrew-formula.sh:15">
P2: The script downloads a release asset named `checksums.txt` but then reads `/tmp/kelos-checksums.txt`. Since the checksums file generated by the release process is `checksums.txt`, the parsing loop will fail when the file isn’t renamed. This will abort the formula update.</violation>
</file>
<file name=".github/workflows/release.yaml">
<violation number="1" location=".github/workflows/release.yaml:99">
P2: Release workflow pushes from a detached HEAD on tag builds; `git push` will fail or not update main, so the Homebrew formula update won’t land.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/release.yaml">
<violation number="1" location=".github/workflows/release.yaml:99">
P1: Tag-triggered workflow may fail to push formula updates because `git push origin main` requires a local `main` ref that is not guaranteed in tag checkout context.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
b347492 to
5e57e3d
Compare
|
I fixed a conflict and squashed the commits. Ready to review |
|
Alright, fixed verify issues and amended the the commit. NOW it is ready for review and merge. Beware that this PR needs to be merged before so the tap repo is as expected: kelos-dev/homebrew-tap#1 Additionally you need to set up a PAT that is allowed to make changes to the tap repo and give it to the kelos repo as a TAP_GITHUB_TOKEN secret: It would be more elegant if we would use goreleaser but that would require more work because it would interfer with your existing build and release notes automation. I therfor decided to not go that way. Hope to be able to install and especially update (that's the bigger value) kelos with homebrew soon. |
|
/kelos review |
|
🤖 Kelos Task Status Task |
gjkim42
left a comment
There was a problem hiding this comment.
can we update the release note?
It's mixed up with cubic's comment.
There was a problem hiding this comment.
🤖 Kelos Reviewer Agent @gjkim42
Review Summary
Verdict: REQUEST CHANGES
Scope: Adds Homebrew formula, release workflow integration, and update script for automatic tap updates on release.
Findings
Correctness
- README.md:146 —
brew tapcommand points to a nonexistent repo.brew tap kelos-dev/kelosresolves togithub.com/kelos-dev/homebrew-kelos, which does not exist. The actual tap repo iskelos-dev/homebrew-tap, so the correct command isbrew tap kelos-dev/tap. This would cause the documented installation instructions to fail for every user. - Formula/kelos.rb:35 — Dead variable
binary_name. The variablebinary_name = "kelos-#{OS.kernel_name.downcase}-#{Hardware::CPU.arch}"is computed but never referenced. The next line usesDir.glob("kelos-*").firstinstead. Remove the unused variable or use it.
Workflow
- .github/workflows/release.yaml:14–15 — Unnecessary bot guard. The
if: github.actor != 'kelos-dev[bot]'condition on the release job can never trigger because the bot only pushes tokelos-dev/homebrew-tap(a different repo), not tokelos-dev/kelos. It won't prevent any real double-build scenario. Not harmful, but misleading — consider removing it or adding a comment explaining the intended future scenario. - .github/workflows/release.yaml — Formula updated before release is published. The release is created with
--draft, and the formula update runs in the same job immediately after. This means the formula (with real checksums and version) is pushed to the tap repo while the release is still a draft. During this window,brew install keloswould fail with a 404 because draft release assets aren't publicly downloadable. Consider either: (a) moving the formula update to a separate workflow triggered byrelease: published, or (b) publishing the release before updating the formula.
Code Quality
- Formula/kelos.rb:6 — Version will include
vprefix. TheVERSIONstep outputsv1.2.3, so the formula will haveversion "v1.2.3". Homebrew convention is bare version numbers (1.2.3). Consider stripping the prefix in the formula (version "1.2.3") and adjusting the URL to...download/v#{version}/.... This avoids potential issues with Homebrew's version comparison logic.
Suggestions
- The formula test (
assert !output.empty?) is weak — considerassert_match "kelos", outputor matching the version string for a more meaningful check. - Consider adding
trap 'rm -f /tmp/checksums.txt' EXITat the top ofhack/update-homebrew-formula.shto clean up the downloaded checksums file.
/kelos needs-input
|
Requested changes should be delivered with 0916609 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds Homebrew installation support for Kelos, allowing users to install and manage Kelos via:
This improves the installation experience and makes Kelos discoverable through a widely-used package manager.
This PR includes:
The formula is automatically updated with correct checksums and version on every release, requiring no manual intervention.
Which issue(s) this PR is related to:
Fixes #817
Special notes for your reviewer:
if: github.actor != 'kelos-dev[bot]')update-homebrew-formula.shscript handles downloading checksums from the GitHub release and updating the formulaDoes this PR introduce a user-facing change?
Yes - a new installation method.