Skip to content

Conversation

@julienrbrt
Copy link
Member

@julienrbrt julienrbrt commented Jan 28, 2026

Overview

We must tag following the go tagging norm, so git tag /path/to/go.mod, so we shouldn't prepend ./apps to the path in the workflow: https://github.com/evstack/ev-node/actions/runs/21438410740/job/61734871603

@gemini-code-assist
Copy link
Contributor

Note

Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 28, 2026

The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedJan 28, 2026, 1:00 PM

@claude
Copy link
Contributor

claude bot commented Jan 28, 2026

Claude finished @julienrbrt's task in 2m 12s —— View job


Comprehensive Code Review

I've completed a thorough review of the release workflow changes. Overall, the changes correctly implement Go module tagging conventions and make important improvements. Here's my detailed analysis:


Summary

Verdict: ✅ LGTM with Minor Suggestions

The PR successfully fixes the Go tagging issue by removing the hardcoded apps/ prefix, which now allows tags like apps/testapp/v0.4.0 to work correctly with Go's module path conventions. The workflow also gains valuable manual trigger capability.


Detailed Review

1. Code Quality & Architecture ✅

Strengths:

  • Correct Go Tagging Implementation: The removal of the apps/ prefix on lines 45-46, 49-50, 54-55, 58, and 64 correctly aligns with Go module conventions where tags should match the module path exactly (.github/workflows/release.yml:45-64)
  • Clear Separation of Concerns: Two-job structure (parse-tagbuild-and-push) with well-defined outputs is clean and maintainable
  • Good Variable Naming: APP_PATH, VERSION, IMAGE_NAME, DOCKERFILE are descriptive and consistent

Observations:

  • The tag pattern **/v*.*.* (line 5) is flexible and correctly matches multi-segment paths
  • Removal of the separate goreleaser.yml reduces complexity (though I note this is a significant architectural change - see section 6 below)

2. Error Handling & Resilience ✅

Strengths:

  • Comprehensive Validation: Lines 49-58 validate both directory existence and Dockerfile presence before proceeding
  • Clear Error Messages: Error messages include the actual paths being checked, making debugging easier
  • Early Failure: The workflow fails fast if validation checks don't pass

Suggestions:

  1. Add Tag Format Validation (.github/workflows/release.yml:31-38):

    # Validate tag format before processing
    if [[ ! "$TAG" =~ ^apps/.+/v[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
      echo "::error::Tag '$TAG' does not match expected format: apps/*/v*.*.*"
      exit 1
    fi

    This would catch malformed tags early and provide clearer feedback.

  2. Validate go.mod Exists (.github/workflows/release.yml:49-58):

    # Verify this is a valid Go module
    if [ ! -f "$APP_PATH/go.mod" ]; then
      echo "::error::go.mod not found in '$APP_PATH/' - not a valid Go module"
      exit 1
    fi

    Since this is fixing Go module tagging, verifying a go.mod exists would add an extra safety layer.

3. Security 🔒

Strengths:

  • Least Privilege Permissions: Top-level permissions: {} (line 13) denies all by default, with minimal grants per job
  • Scoped Job Permissions:
    • parse-tag: Only contents: read (line 19)
    • build-and-push: Only contents: read + packages: write (lines 73-74)
  • No Secret Exposure: Uses built-in GITHUB_TOKEN instead of custom secrets

Improvement from Previous Version:

  • ✅ Removed permissions: write-all that was in the deleted goreleaser.yml (major security improvement)
  • ✅ Removed dependency on GORELEASER_TOKEN custom secret

Note on Deleted goreleaser.yml:
The removed workflow had permissions: write-all (line 21 in deleted file), which is a security anti-pattern. The new approach is significantly more secure.

4. Performance & Resource Efficiency ⚡

Strengths:

  • Multi-platform Builds: Line 95 builds for linux/amd64,linux/arm64 which is excellent for compatibility
  • Efficient Checkout: Uses actions/checkout@v6 without fetch-depth: 0 since full history isn't needed
  • Docker Buildx: Uses buildx (line 79-80) for efficient multi-platform builds with caching support

Potential Optimization:
Consider adding build cache configuration to speed up rebuilds:

- name: Build and push Docker image
  uses: docker/build-push-action@v6
  with:
    context: .
    file: ${{ needs.parse-tag.outputs.dockerfile }}
    push: true
    platforms: linux/amd64,linux/arm64
    cache-from: type=gha
    cache-to: type=gha,mode=max
    tags: |
      ghcr.io/${{ github.repository_owner }}/${{ needs.parse-tag.outputs.image-name }}:${{ needs.parse-tag.outputs.version }}
      ghcr.io/${{ github.repository_owner }}/${{ needs.parse-tag.outputs.image-name }}:latest

5. Testing & Reliability 🧪

Strengths:

  • Manual Testing Capability: The new workflow_dispatch trigger (lines 6-11) allows testing without creating tags - this is excellent for validation
  • Idempotent: Can be re-run safely since it uses explicit version tags

Concerns:

  1. No Validation Tests: The workflow doesn't run any tests before building/pushing Docker images
  2. No Dry-Run Mode: Consider adding a dry_run input to workflow_dispatch to test without pushing

Suggestion - Add Test Job (optional but recommended):

test:
  name: Run Tests
  runs-on: ubuntu-latest
  needs: parse-tag
  steps:
    - uses: actions/checkout@v6
    - uses: actions/setup-go@v6
      with:
        go-version-file: ${{ needs.parse-tag.outputs.app-path }}/go.mod
    - name: Run tests
      run: cd ${{ needs.parse-tag.outputs.app-path }} && go test ./...

build-and-push:
  needs: [parse-tag, test]  # Add test dependency
  # ... rest of job

6. Documentation & Developer Experience 📚

Strengths:

  • Excellent Comments:
    • Line 5: Clear example tags in comment
    • Line 9: Explicit format requirement
    • Lines 31-38: Clear logic flow comments
  • Descriptive Notice: Line 66 provides a helpful summary of what's being built

Missing Documentation:

  1. No Migration Guide: This is a breaking change - developers need to know:

    • Old tags: evm/single/v0.2.0
    • New tags: apps/evm/single/v0.2.0 (must include apps/ prefix)
    • What happened to goreleaser functionality?
  2. PR Description Could Be Clearer: The PR description mentions the fix but doesn't explain:

    • Why was goreleaser.yml removed entirely?
    • What happens to binary releases (goreleaser created these)?
    • Are we only building Docker images now?

Suggestion: Add a section to the PR description:

## Breaking Changes

- Tags must now include the `apps/` prefix to match Go module paths
  - ❌ Old: `testapp/v0.4.0`
  - ✅ New: `apps/testapp/v0.4.0`

## Architectural Changes

- Removed `goreleaser.yml` workflow (consolidated into `release.yml`)
- Release process now only builds and pushes Docker images
- Binary releases via goreleaser are no longer automated

Critical Questions

❓ Goreleaser Removal

The deletion of goreleaser.yml is a significant change that's not mentioned in the PR description. This workflow was responsible for:

  • Creating prebuilt binaries (make prebuilt-binary)
  • Attaching binaries to GitHub releases
  • Validating .goreleaser.yaml config

Questions for PR author:

  1. Is the goreleaser functionality intentionally being removed, or should it be integrated into the new workflow?
  2. Do users/consumers of ev-node rely on the prebuilt binaries from GitHub releases?
  3. If yes, should we add a job to build and upload binaries alongside the Docker images?

❓ Tag Pattern Mismatch Risk

The tag pattern **/v*.*.* will match ANY path with a version, including potentially invalid paths like random/path/v1.0.0. While the validation checks will catch this, consider if a more restrictive pattern would be better:

tags:
  - "apps/*/v*.*.*"        # Matches only one level deep
  - "apps/*/*/v*.*.*"      # Matches two levels deep

This would prevent accidental triggers from incorrectly placed tags.


Security Improvements from Base

Major security wins:

  1. Removed permissions: write-all (extremely dangerous)
  2. Removed custom GORELEASER_TOKEN secret dependency
  3. Implemented least-privilege permission model
  4. Uses built-in GITHUB_TOKEN with minimal scopes

Final Recommendations

Must Address:

  1. Document the breaking change in PR description (tag format now requires apps/ prefix)
  2. Clarify goreleaser removal - is this intentional? Do we need binary releases?

Should Consider:

  1. Add tag format validation to catch malformed tags early
  2. Add go.mod existence validation
  3. Add test job before building Docker images
  4. Add build caching for faster rebuilds

Nice to Have:

  1. Restrict tag pattern to apps/*/v*.*.* or apps/*/*/v*.*.*
  2. Add dry-run mode for manual testing
  3. Update repository documentation about the new tagging convention

Conclusion

The core fix (removing apps/ prefix) is correct and necessary for Go module compatibility. The workflow is secure and well-structured. However, the removal of goreleaser functionality needs clarification, as it may be an unintended side effect.

Recommendation: Approve after addressing the goreleaser question and adding release notes about the breaking tag format change.

@julienrbrt julienrbrt requested a review from auricom January 28, 2026 12:47
@codecov
Copy link

codecov bot commented Jan 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.59%. Comparing base (4cf1c92) to head (679c423).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3027      +/-   ##
==========================================
- Coverage   55.66%   55.59%   -0.07%     
==========================================
  Files         116      116              
  Lines       11477    11477              
==========================================
- Hits         6389     6381       -8     
- Misses       4389     4395       +6     
- Partials      699      701       +2     
Flag Coverage Δ
combined 55.59% <ø> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@julienrbrt julienrbrt merged commit 192c3a3 into main Jan 28, 2026
24 checks passed
@julienrbrt julienrbrt deleted the julien/ci branch January 28, 2026 13:04
alpe added a commit that referenced this pull request Jan 28, 2026
* main:
  ci: strip app prefix (#3028)
  ci: fix release workflow (#3027)
  chore: prep apps (#3025)
  build: fix docker-compose for evm (#3022)
  chore: prepare execution release (#3021)
  chore: prep changelog (#3020)
  refactor(e2e): extract shared test helpers to DockerTestSuite (#3017)
  feat: High availabilty via RAFT (#2987)
  chore: bump to core rc.1 (#3018)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants