feat: create github action to run gevals - #31
Conversation
Signed-off-by: Calum Murray <cmurray@redhat.com>
WalkthroughAdds a reusable composite GitHub Action to run MCP evaluations with gevals and three CI workflows (release, prerelease, nightly). Also adds Makefile release targets, release docs, changelog, and .gitignore entries for release artifacts. Changes
Sequence Diagram(s)sequenceDiagram
participant WF as GitHub Workflow
participant Action as gevals-action
participant Installer as Installer
participant Eval as gevals
participant Parser as Results Parser
participant Uploader as Artifact Uploader
WF->>Action: invoke with inputs
Action->>Installer: detect GOOS/GOARCH & set suffix/extension
alt gevals-version == latest/main
Installer->>Installer: clone repo & build binaries
else specified version
Installer->>Installer: try download prebuilt binaries
Installer->>Installer: on failure -> clone & build from source
end
Installer->>Action: export `gevals-path` & `agent-path`
Action->>Eval: run `gevals eval` (eval-config, filters, flags)
Eval->>Parser: produce results file (gevals-*-out.json)
alt jq available
Parser->>Parser: compute totals, passes, pass rates, threshold checks
else
Parser->>Action: emit limited metrics + warning
end
opt upload-artifacts == true
Action->>Uploader: upload results & artifacts
end
Action->>WF: set workflow outputs and exit (respecting fail-on-error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/actions/gevals-action/action.yaml(1 hunks)
🔇 Additional comments (2)
.github/actions/gevals-action/action.yaml (2)
417-417: Verify GitHub Actions condition syntax forifstatement.Line 417 uses
if: always() && inputs.upload-artifacts == 'true'which mixes context functions. While this likely works, the syntax is unconventional compared to typical GitHub Actions patterns.Verify that this condition behaves as expected: always run this step AND only if
upload-artifactsinput is'true'. If needed, clarify or adjust for consistency with GitHub Actions documentation.
164-164: No changes needed — Go 1.24.x is already released and supported.Go 1.25 is the latest stable version as of November 2025, and GitHub Actions supports Go 1.24. Go 1.24.x is not a future version; it has been released and is available on runners. The pinned version is both intentional and safe for use in GitHub Actions workflows.
| echo "=== Evaluation Results ===" | ||
|
|
||
| # Find results file | ||
| RESULTS_FILE=$(ls gevals-*-out.json 2>/dev/null | head -1 || echo "") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use find instead of ls for reliable results file discovery.
Line 287 uses ls gevals-*-out.json | head -1 to find the results file. The ls command with glob patterns can produce non-deterministic ordering, and using head -1 arbitrarily selects the first match. If multiple result files are present (e.g., from parallel test runs), behavior is unpredictable.
Replace with:
RESULTS_FILE=$(find . -name 'gevals-*-out.json' -type f | sort | head -1)Or, better yet, if the action always produces a single well-named result file, explicitly specify the expected filename:
RESULTS_FILE="${PWD}/gevals-results-out.json" # if naming is deterministic🤖 Prompt for AI Agents
.github/actions/gevals-action/action.yaml around line 287: the script uses "ls
gevals-*-out.json | head -1" which yields nondeterministic results when multiple
matching files exist; replace this with a deterministic discovery method such as
using find to locate files, sort them and take the first match, or even better,
reference the single expected filename directly if it is deterministic (e.g.,
set RESULTS_FILE to the known path) so the action consistently picks the correct
results file.
Signed-off-by: Calum Murray <cmurray@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.github/workflows/release.yaml (2)
56-66: CHANGELOG validation is good, but error message could be more helpful.The validation on line 59 checks for CHANGELOG.md section format correctly. However, the error output on lines 62-63 could be improved to show more context about what sections actually exist.
Lines 62-63 attempt to grep for version sections but may produce no output. Make this more robust:
- name: Validate CHANGELOG has version section run: | VERSION="${{ env.VERSION }}" if ! grep -q "## \[${VERSION}\]" CHANGELOG.md; then echo "Error: CHANGELOG.md must contain a section for version ${VERSION}" echo "Expected format: ## [${VERSION}]" echo "Found version sections:" grep "^## \[" CHANGELOG.md || echo " (no version sections found)" exit 1 fi
87-91: Consider more comprehensive test execution.Line 90 runs
go test ./...which tests all packages. While this is good, consider if you need:
- Specific test flags (e.g.,
-race,-cover,-timeout)- Coverage thresholds
- Integration test exclusion
- name: Run tests run: | echo "Running all tests with race detection and coverage..." go test -race -cover -timeout 10m ./... echo "All tests passed!".github/workflows/nightly.yaml (1)
52-65: Nightly existence check uses tag but should verify against release.Lines 52-65 check if a nightly already exists for the current commit by looking at the
nightlygit tag. However, there's a potential race condition:
- Line 52-54: Checks if
nightlytag points to current commit- Lines 86-99: Later, the job deletes this tag/release
- Line 112: Creates a new release
If two builds run concurrently, the second build might see the first build's nightly tag (line 52), decide to skip (line 58), but then the first build's cleanup (line 86+) deletes it, leaving no nightly for that commit.
Consider removing the "skip if nightly exists for same commit" logic to ensure a nightly is always created for the current commit. Or add locking/retries:
- name: Check for unreleased commits id: check_commits run: | # Find the latest release tag (any x.y.z release) LATEST_RELEASE=$(git tag -l 'v*.*.*' --sort=-version:refname | grep -v prerelease | grep -v nightly | head -n1) if [ -z "$LATEST_RELEASE" ]; then COMMITS_SINCE_RELEASE=$(git rev-list HEAD --count) else COMMITS_SINCE_RELEASE=$(git rev-list ${LATEST_RELEASE}..HEAD --count) fi if [ "$COMMITS_SINCE_RELEASE" -eq "0" ]; then echo "No new commits since latest release, skipping nightly" echo "SKIP_NIGHTLY=true" >> "$GITHUB_ENV" exit 0 fi NIGHTLY_VERSION="nightly" echo "NIGHTLY_VERSION=$NIGHTLY_VERSION" >> "$GITHUB_ENV" echo "Creating nightly release: $NIGHTLY_VERSION"This removes the duplicate commit check and always creates a nightly if there are new commits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/nightly.yaml(1 hunks).github/workflows/prerelease.yaml(1 hunks).github/workflows/release.yaml(1 hunks)
🔇 Additional comments (3)
.github/workflows/prerelease.yaml (1)
78-102: Verify gh release edit behavior with --tag and --draft flags.Line 101-108 uses
gh release editwith--tagand--draftflags. The behavior of changing a release's tag viagh release editmay not be well-defined or supported. Verify this works as intended by testing manually before merge.Please verify:
- Does
gh release edit <release_name> --tag <new_tag>actually change the release tag?- What is the expected behavior when updating a release from prerelease to prerelease (same state)?
- Test the flow where a prerelease already exists for the version and needs updating.
Consider using a more explicit flow if
gh release edit --tagdoesn't work as expected:if gh release view "$VERSION" >/dev/null 2>&1; then # Delete and recreate, or use a different update strategy fi.github/workflows/nightly.yaml (1)
28-28: The grep pattern on line 28 is correct; the review comment is based on incorrect assumptions.The regex
grep -v '^nightly'is the proper approach becausegit tag -loutputs one tag per line, making^the correct anchor to match tags starting with "nightly". The pattern reliably filters prerelease and nightly tags as intended. The suggestion to remove the^anchor or use an explicit pattern is unnecessary—the current implementation works correctly.Likely an incorrect or invalid review comment.
.github/workflows/release.yaml (1)
93-118: The original review comment is incorrect and should be disregarded.The
--tagflag does exist forgh release editand is a documented feature of the GitHub CLI. It allows changing the git tag associated with an existing release, which means the workflow code at lines 99-108 is valid and will work correctly to convert a prerelease to a final release by renaming its tag.The approach in the original code—using
gh release editwith--tagto move the prerelease release to the final version tag—is the correct and recommended method for this use case.Likely an incorrect or invalid review comment.
|
@matzew @manusa the release workflows seem to work, they produced: https://github.com/Cali0707/gevals/releases/tag/v0.0.0 What are your opinions on merging this, and then if needed we can do follow up PRs to debug the reusable action itself? |
Signed-off-by: Calum Murray <cmurray@redhat.com>
manusa
left a comment
manusa
left a comment
There was a problem hiding this comment.
Nice!
Overall looks good, adding just some comments and personal impressions.
action.yaml
TIL about composite actions https://docs.github.com/en/actions/tutorials/create-actions/create-a-composite-action
Seems quite a neat approach for simpler GitHub actions.
For this case, eventually something more complex might be needed though (this code is untestable and there's some complex logic going on).
For future iterations:
- Since in some scenario (version != latest) the binary is downloaded from the GitHub release, GH token will be necessary to avoid GitHub API request limits.
Release workflows
Take this with a grain of salt as these are my highly opinionated comments. Maybe you have other conventions in the rest of your projects, so ignore my comments if they are not applicable.
For the release workflows, I'd try to move some of the logic into the Makefile to avoid repetition across the different release pipelines and to be able to release manually in the event of CI issues or downtime.
I also see that the version is sort of calculated using a branch and a CHANGELOG.md file, but I don't see this file in the repository.
I'm assuming this is the intended format https://github.com/Cali0707/gevals/blob/5017e551ba9722f6191a860f2d042a81fa93be1a/CHANGELOG.md.
I'm not a friend of calculating the versions automatically, since versioning (especially semantic versioning) drives a lot of expectations for the end users.
In this case, I see that the release is triggered by pushing to a given branch, I would switch to release on tag push (where tag v*). This would also remove the requirement to calculate the z version.
Signed-off-by: Calum Murray <cmurray@redhat.com>
- releases are now triggered by git tags - most of the build logic is now done through make targets - action correctly installs the releases Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
|
Hey @manusa - tried to address all your feedback, thanks for taking a look! @manusa @nader-ziada can you review? This should let us start getting CI evals in the kubernetes-mcp-server |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/release.yaml (1)
61-78: Complex changelog extraction logic is duplicated across workflows.Lines 64–78 use the same complex sed/awk pipeline found in prerelease.yaml (lines 56–73) and nightly.yaml (lines 72). This brittle logic depends on specific CHANGELOG formatting and is difficult to maintain.
Consider extracting this into a reusable shell script (
.github/scripts/extract-changelog.sh) to eliminate duplication and centralize maintenance..github/workflows/prerelease.yaml (1)
48-73: Complex and repeated changelog extraction logic is fragile.The sed/awk pipeline on lines 56–73 is complex, depends on specific formatting assumptions, and is duplicated identically across all three workflows (release.yaml:64, prerelease.yaml:56, nightly.yaml:72).
This logic should be extracted into a reusable shell script (
.github/scripts/extract-changelog.sh) and called from all workflows to eliminate duplication and simplify maintenance..github/workflows/nightly.yaml (1)
67-84: Complex and repeated changelog extraction logic is fragile.The sed/awk pipeline on lines 72–84 is identical to that in release.yaml and prerelease.yaml. This duplication should be extracted into a reusable script (
.github/scripts/extract-changelog.sh) to simplify maintenance.
🧹 Nitpick comments (2)
RELEASING.md (2)
50-50: Minor: Capitalize "GitHub" in user-facing text.Line 50 uses lowercase "github" which should be "GitHub" per the official branding.
Apply this diff:
- You can do this through the github UI (on the releases page), or by git with the following commands: + You can do this through the GitHub UI (on the releases page), or by git with the following commands:
142-142: Minor: Use hyphenated compound adjective before noun.Line 142 uses "backwards compatible" as a compound adjective; it should be "backward-compatible" when modifying the following noun.
Apply this diff:
- - **MINOR** (x.Y.0): New functionality, backwards compatible + - **MINOR** (x.Y.0): New functionality, backward-compatible
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/actions/gevals-action/action.yaml(1 hunks).github/workflows/nightly.yaml(1 hunks).github/workflows/prerelease.yaml(1 hunks).github/workflows/release.yaml(1 hunks).gitignore(1 hunks)CHANGELOG.md(1 hunks)Makefile(2 hunks)RELEASING.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 62-62: Missing required phony target "all"
(minphony)
[warning] 62-62: Missing required phony target "test"
(minphony)
🪛 LanguageTool
RELEASING.md
[uncategorized] ~50-~50: The official name of this software platform is spelled with a capital “H”.
Context: ...n tag** You can do this through the github UI (on the releases page), or by git wi...
(GITHUB)
[uncategorized] ~142-~142: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...- MINOR (x.Y.0): New functionality, backwards compatible - PATCH (x.y.Z): Bug fixes, backwar...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🔇 Additional comments (16)
.gitignore (1)
38-41: LGTM: Release artifact ignores are correctly added.The new patterns properly exclude transient release artifacts generated by the new Makefile targets and CI workflows.
RELEASING.md (1)
1-174: LGTM: Comprehensive and clear release documentation.The guide provides excellent step-by-step instructions, aligns well with the automated workflows in this PR, and covers security practices (cosign signing) and all three release types. The documentation will help maintainers and contributors understand the release process.
Makefile (2)
4-12: LGTM: Release variables and clean target correctly configured.The new variables use the
?=override syntax appropriately, allowing CI workflows to pass GOOS/GOARCH at invocation time. The clean target now properly removes release artifacts.
25-65: LGTM: Release targets are well-structured and platform-aware.
build-releasecorrectly handles Windows binary extensions (.exe) and uses optimized flags (trimpath, ldflags).package-releasecreates deterministic ZIP archives with proper platform naming.sign-releaseuses cosign with bundle format (aligns with security practices in RELEASING.md).releasetarget cleanly orchestrates the workflow.The targets properly support cross-compilation via GOOS/GOARCH environment variables, which is essential for the CI workflows.
.github/workflows/release.yaml (2)
29-47: LGTM: Version derivation and validation are correct.Tag format validation via regex correctly enforces vX.Y.Z format and rejects pre-releases. Deriving version from either push tag or manual input is well-handled. The flow gates subsequent jobs appropriately.
115-150: LGTM: Build and upload workflow is well-structured.The matrix strategy correctly builds for all platform/architecture combinations (6 total). Cosign signing and artifact upload via
gh release uploadwith--clobberis appropriate. Permissions are correctly scoped (contents: write, id-token: write for keyless signing)..github/workflows/prerelease.yaml (2)
28-47: LGTM: Pre-release version validation is correct.The regex correctly enforces vX.Y.Z-rc.N format. Deriving from push tag or manual input is properly handled. The structure cleanly separates concerns between determine-version and build-and-upload jobs.
104-153: LGTM: Pre-release build and upload workflow is well-structured.Matrix strategy, cosign signing, and artifact upload are identical to the stable release workflow and correctly implemented. The
--prereleaseflag on line 99 properly marks the release in GitHub..github/workflows/nightly.yaml (2)
24-60: LGTM: Unreleased commit detection is well-reasoned.The logic correctly skips nightly creation when no new commits exist since the last stable release, and also skips if a nightly already exists for the current commit. This prevents unnecessary CI runs and redundant releases.
119-166: LGTM: Build and upload workflow mirrors stable/pre-release with appropriate conditionals.The matrix strategy and artifact upload are correctly implemented. The
ifcondition on line 121 properly skips this job when nightly creation is skipped..github/actions/gevals-action/action.yaml (6)
161-165: LGTM: Go version selection is appropriate.Go 1.24 was released in February 2025, and it is supported with full support until May 2026. Using
1.24.xis a stable, current choice that ensures compatibility with the latest toolchain features while maintaining point-release flexibility.
293-314: LGTM: Command construction safely uses bash arrays.The action builds commands using a bash array (line 294) and executes it directly without
eval(line 312). This prevents shell injection vulnerabilities from user inputs liketask-filterand is a significant security improvement over string-based construction.
209-229: LGTM: Git clone fallback includes explicit error handling.Lines 209–229 properly attempt to download pre-built binaries and fall back to building from source if the release doesn't exist. Error messages are clear and distinguish between download failures and git clone failures, helping users understand what went wrong.
319-325: LGTM: Results file discovery is deterministic.Line 321 uses
ls -t(sorts by modification time, newest first) to ensure deterministic selection of the latest results file. This is better than arbitraryhead -1without sorting.
336-362: LGTM: Assertion counting is dynamic and future-proof.Lines 336–348 use jq to dynamically count assertion results without hard-coding assertion types. This approach adapts automatically if gevals adds new assertion types, avoiding silent counting errors and making the action more maintainable.
350-369: LGTM: Arithmetic calculations use portable awk instead of bc.Lines 350–354 use
awkfor pass-rate calculations and threshold comparisons instead ofbc. This improves portability across platforms (including Windows runners) and maintains consistent arithmetic mode throughout.
Signed-off-by: Calum Murray <cmurray@redhat.com>
|
in general looks good to me, agree with the @manusa comments. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/nightly.yaml (2)
71-88: Changelog extraction logic is fragile to format changes.The sed/awk pipeline on line 76 is complex and tightly coupled to CHANGELOG.md formatting. If the changelog structure deviates (e.g., subsection names, indentation, or section delimiters), the extraction silently fails and defaults to a generic message. Consider:
- Adding inline validation/logging to debug what was actually extracted.
- Testing the extraction logic independently to catch format regressions early.
- Documenting the expected CHANGELOG.md structure to prevent accidental breaks.
You could extract this logic into a helper script (e.g.,
scripts/extract-changelog.sh) to make it testable and more maintainable:#!/bin/bash # Extract Unreleased section from CHANGELOG.md # Expected format: ## [Unreleased] followed by ### subsections and - bullet points CHANGELOG_CONTENT=$(sed -n '/## \[Unreleased\]/,/## \[/p' CHANGELOG.md | sed '$d' | tail -n +2 | awk '/^### /{section=$0; items=""; next} /^( *)?- /{items=items $0 "\n"; next} /^$/ && items{print section "\n" items; items=""}' | sed '/^$/d') if [ -z "$CHANGELOG_CONTENT" ]; then echo "Warning: No changelog content extracted. Defaulting to generic message." >&2 echo "See CHANGELOG.md for details." else echo "$CHANGELOG_CONTENT" fiThen call it in the workflow:
- name: Extract changelog for nightly id: changelog if: env.SKIP_NIGHTLY != 'true' run: | CHANGELOG_CONTENT=$(./scripts/extract-changelog.sh) { echo 'CHANGELOG_BODY<<EOF' echo "$CHANGELOG_CONTENT" echo 'EOF' } >> "$GITHUB_ENV"
162-174: Verify artifact upload logic handles missing artifacts gracefully.The upload loop (lines 167-172) iterates over
dist/*.zipanddist/*.bundlefiles. If no artifacts are generated (e.g., due to a build failure that was not caught), the loop will silently produce no uploads. Consider:
- Failing the step if expected artifacts are missing.
- Logging which artifacts were uploaded for debugging.
- name: Upload assets to nightly release run: | VERSION="${{ needs.check-and-create-nightly.outputs.nightly_version }}" # Collect all artifacts to upload artifacts=(dist/*.zip dist/*.bundle) if [ ${#artifacts[@]} -eq 0 ] || [ ! -e "${artifacts[0]}" ]; then echo "ERROR: No artifacts found in dist/ directory" exit 1 fi # Upload all artifacts for file in "${artifacts[@]}"; do if [ -f "$file" ]; then echo "Uploading $file..." gh release upload "$VERSION" "$file" --clobber fi done echo "Upload complete: uploaded ${#artifacts[@]} artifacts" env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/nightly.yaml(1 hunks)
🔇 Additional comments (1)
.github/workflows/nightly.yaml (1)
9-11: Excellent response to previous feedback on error handling and concurrency control.The addition of the top-level concurrency block (lines 9-11) and the replacement of silent error suppression with explicit error handling (lines 103-106, replacing the previous
|| true) are well-implemented. The workflow now prevents concurrent nightly jobs from racing on tag/release deletion, and failures during tag push are properly surfaced with a non-zero exit code.Also applies to: 103-106
| - name: Build release artifacts | ||
| env: | ||
| GOOS: ${{ matrix.goos }} | ||
| GOARCH: ${{ matrix.goarch }} | ||
| VERSION: ${{ needs.check-and-create-nightly.outputs.nightly_version }} | ||
| run: make build-release package-release | ||
|
|
||
| - name: Sign release artifacts | ||
| env: | ||
| GOOS: ${{ matrix.goos }} | ||
| GOARCH: ${{ matrix.goarch }} | ||
| run: make sign-release |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that Make targets exist and are compatible with the matrix variables.
The workflow invokes make build-release, make package-release, and make sign-release with GOOS, GOARCH, and VERSION environment variables. Ensure:
- These targets exist in the Makefile and accept the injected environment variables.
- The targets behave correctly when
VERSION=nightly. - Artifact output paths (
dist/*.zip,dist/*.bundle) match the Makefile's actual output structure.
Run the following script to verify the Make targets and their compatibility with the environment variables:
🏁 Script executed:
#!/bin/bash
# Description: Verify that Make targets exist and are documented.
# Check for Makefile and look for the required targets
if [ ! -f "Makefile" ]; then
echo "ERROR: Makefile not found at repository root"
exit 1
fi
echo "=== Checking for required Make targets ==="
for target in build-release package-release sign-release; do
if grep -q "^${target}:" Makefile; then
echo "✓ Target '${target}' found"
else
echo "✗ Target '${target}' NOT found"
fi
done
echo ""
echo "=== Checking Makefile for GOOS/GOARCH/VERSION usage ==="
rg -n "GOOS|GOARCH|VERSION" Makefile | head -20Length of output: 1793
---
🏁 Script executed:
```shell
#!/bin/bash
# Check how VERSION variable is actually used in the build-release target
echo "=== Full build-release target definition ==="
sed -n '/^build-release:/,/^[a-z-]*:/p' Makefile | head -20
echo ""
echo "=== Checking if VERSION is passed via ldflags ==="
grep -n "ldflags" Makefile
echo ""
echo "=== Checking artifact naming in all targets ==="
grep -n "dist/" Makefile | grep -E "(zip|bundle|exe|\.zip)"
Length of output: 280
🏁 Script executed:
#!/bin/bash
# Check how VERSION variable is actually used in the build-release target
echo "=== Full build-release target definition ==="
sed -n '/^build-release:/,/^[a-z-]*:/p' Makefile | head -25
echo ""
echo "=== Checking if VERSION is passed via ldflags ==="
grep -A10 "build-release:" Makefile | grep -E "(ldflags|VERSION)"
echo ""
echo "=== All VERSION references in Makefile ==="
grep -n "VERSION" MakefileLength of output: 1445
🏁 Script executed:
#!/bin/bash
# Check all three targets for VERSION usage
echo "=== package-release target ==="
sed -n '/^package-release:/,/^[a-z-]*:/p' Makefile | head -20
echo ""
echo "=== sign-release target ==="
sed -n '/^sign-release:/,/^[a-z-]*:/p' Makefile | head -20
echo ""
echo "=== Summary: Does any target use VERSION variable? ==="
if grep -q "VERSION" Makefile | grep -v "^VERSION"; then
echo "VERSION is used in targets"
else
echo "VERSION is NOT used in any target (only defined, never referenced in commands)"
fiLength of output: 1255
🏁 Script executed:
#!/bin/bash
# Check the workflow to understand intent for VERSION/nightly_version
echo "=== Checking nightly workflow ==="
cat .github/workflows/nightly.yaml | grep -A5 -B5 "nightly_version\|VERSION"Length of output: 2248
VERSION environment variable is passed by the workflow but not consumed by any Make target.
The Make targets build-release, package-release, and sign-release exist and correctly accept GOOS and GOARCH. However, the VERSION variable set by the workflow is not used by any of these targets. The Makefile defines VERSION ?= dev but never references it in any command.
build-release: Does not pass VERSION to ldflags or include it in output filenamespackage-release: Artifact naming uses only GOOS/GOARCH (e.g.,binary-linux-amd64.zip)sign-release: Only operates on GOOS/GOARCH-based filenames
The targets function correctly with the matrix variables, but the nightly_version from the workflow is silently discarded during the build process. If versioning is intentional, consider embedding the version via ldflags (e.g., -ldflags="-X main.Version=$(VERSION)") or including it in artifact paths.
🤖 Prompt for AI Agents
.github/workflows/nightly.yaml around lines 149-160: the workflow exports
VERSION but the Make targets ignore it; update the CI and Makefile so VERSION is
consumed — pass VERSION through to the make invocations (or export it in the run
step) and update Makefile targets: for build-release include VERSION in the
build ldflags (e.g., set -ldflags to inject main.Version from $(VERSION)), for
package-release include VERSION in artifact filenames/paths (e.g.,
binary-$(VERSION)-$(GOOS)-$(GOARCH).zip), and for sign-release operate on those
versioned artifact names; keep the existing VERSION ?= dev default in the
Makefile for local builds.
Yeah that makes sense, for all the success/fail logic extraction I actually would prefer to add that to the CLI itself, and have that output a single summary JSON somehow which we can use e.g. I'll try and add more of the setup work from the action into make targets for now |
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Cali0707
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/actions/gevals-action/action.yaml (1)
324-324: Results file discovery could be more robust usingfindinstead ofls.Line 324 uses
ls -t gevals-*-out.json | head -1which, while improved over plainls, still relies on command substitution and glob pattern matching. For more deterministic behavior across edge cases, consider usingfindwith explicit sorting:RESULTS_FILE=$(find . -name 'gevals-*-out.json' -type f -printf '%T@ %p\n' | sort -rn | head -1 | cut -d' ' -f2-)Alternatively, if the results file name is always deterministic:
RESULTS_FILE="${PWD}/gevals-results-out.json"The current approach works for typical cases, but
findis more robust for unusual filenames or special characters. This is an optional improvement for future consideration..github/workflows/nightly.yaml (2)
32-32: Simplify redundant tag filtering.The
grep -v '^nightly'at line 32 is redundant sincegit tag -l 'v*.*.*'already excludes tags not matching thev*.*.*pattern (andnightlydoes not match this pattern). Removing it reduces noise without changing behavior.- LATEST_RELEASE=$(git tag -l 'v*.*.*' --sort=-version:refname | grep -v 'prerelease' | grep -v '^nightly' | head -n1) + LATEST_RELEASE=$(git tag -l 'v*.*.*' --sort=-version:refname | grep -v 'prerelease' | head -n1)
71-88: Consider simplifying changelog extraction logic.The sed/awk pipeline at line 76 is complex and fragile—it will break if the CHANGELOG.md format deviates from the expected structure (e.g., section headers, subsection indentation). While it currently works, maintainability could be improved by extracting this logic into a dedicated script (e.g.,
scripts/extract-changelog.sh) or a simple Go/Python utility. This is a lower priority but worth considering for long-term maintainability, especially if CHANGELOG format evolves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/actions/gevals-action/action.yaml(1 hunks).github/workflows/nightly.yaml(1 hunks)
🔇 Additional comments (8)
.github/actions/gevals-action/action.yaml (7)
1-100: Input/output definitions are well-structured and documented.The interface is clear with sensible defaults and comprehensive descriptions.
104-160: Platform detection is thorough and handles major OS/architecture combinations well.Error messages are clear for unsupported platforms.
161-279: Install logic handles multiple source strategies with proper fallbacks and error messaging.Verification ensures binaries exist before proceeding.
281-290: Verify that gevals binary supports the barehelpcommand.Line 285 invokes
helpas a subcommand, but standard CLI tools often use--helpor-hinstead. If gevals expects a flag rather than a subcommand, this step will fail and block the entire action.Can you confirm that the gevals binary implements
helpas a subcommand? If it uses--helpinstead, this should be updated to${{ steps.install.outputs.gevals-path }} --help.
296-317: Command construction via bash arrays properly eliminates shell injection risks.The approach is safe and correct.
331-410: Results parsing with dynamic assertion counting and jq graceful fallback is well-implemented.The jq queries (lines 339–351) properly avoid hard-coded assertion type enumeration, making the action future-proof for new assertion types. Math operations correctly use awk instead of bc for cross-platform compatibility. Fallback outputs (lines 401–409) prevent undefined outputs when jq is unavailable.
432-440: Artifact upload properly usesalways()and conditional input.The pattern uploads both results and error files, with appropriate
if-no-files-found: warnto prevent step failure..github/workflows/nightly.yaml (1)
149-159: Verify whether VERSION should be embedded in nightly binaries — Make targets currently don't consume VERSION variable.The verification shows that the Make targets (
build-release,package-release,sign-release) do not reference theVERSIONvariable at all. They only useGOOSandGOARCHfor artifact naming, and the build step omits version embedding (ldflags only strip symbols:-s -w).Current state:
- Workflow sets
NIGHTLY_VERSION=nightlybut never passes it to Make- GitHub release is named
nightly, but binaries inside have no version info- Make default
VERSION ?= devis unusedThe suggested fix (adding
VERSIONenv var to the build/sign steps) would not resolve anything unless the Makefile is also updated to consumeVERSION(e.g., via-ldflags="-X main.Version=..."or in artifact naming).Action: Clarify intent — is this design intentional (nightly binaries unversioned) or should the Makefile be updated to embed or include the version in artifacts?
|
lgtm |
manusa
left a comment
manusa
left a comment
There was a problem hiding this comment.
Looks good 👍
For the future I'd still consider moving more parts of the release logic somewhere else so that it can be reused by the different pipelines.
Note that for the previous statement I was referring to the logic performed by the release pipelines to compute the changelog, extract version, verify version, etc.
Yeah that makes sense, for all the success/fail logic extraction I actually would prefer to add that to the CLI itself, and have that output a single summary JSON somehow which we can use e.g. jq in the action to extract to the outputs. But, that is a next step 😄
💯
3 participants
Resolves #23
Note: this is still WIP, I want to test this action manually somewhere before merging it
Summary by CodeRabbit
New Features
Chores
Documentation