Skip to content

Conversation

@julienrbrt
Copy link
Member

Overview

@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

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

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

@claude
Copy link
Contributor

claude bot commented Jan 28, 2026

Claude finished @julienrbrt's task in 1m 18s —— View job


Code Review Complete ✅

Tasks

  • Read the modified workflow file
  • Analyze the PR changes using git diff
  • Review changes for correctness and best practices
  • Provide comprehensive feedback

Summary

This PR strips the apps/ prefix from Docker image names in the release workflow. The change is correct and improves the naming convention for published images.

Before: ev-node-apps-evm-single (for tag apps/evm/single/v0.2.0)
After: ev-node-evm-single (cleaner, removes redundant prefix)

Detailed Review

✅ Code Quality & Correctness

The implementation is clean and follows bash best practices:

Lines 62-65 (.github/workflows/release.yml:62-65):

# Generate image name from app path (strip apps/ prefix, replace / with -)
IMAGE_PATH="${APP_PATH#apps/}"
IMAGE_NAME="ev-node-${IMAGE_PATH//\//-}"
  • Uses POSIX-compliant parameter expansion ${APP_PATH#apps/} to strip the prefix
  • Correctly chains the transformations: strip prefix → replace slashes → prepend ev-node-
  • Comment accurately describes the operation
  • Safe for paths that don't start with apps/ (no-op in that case)

✅ Security & Safety

  • No security concerns introduced
  • No user input is directly interpolated without validation
  • The APP_PATH variable is validated earlier (lines 48-52) to ensure the directory exists
  • Output variables are properly quoted in downstream usage (line 98)

⚠️ Edge Cases & Robustness

Potential issue: The code assumes all tags follow the apps/*/v*.*.* pattern, but the workflow trigger accepts **/v*.*.* (line 5).

tags:
  - "**/v*.*.*"  # This allows tags like foo/bar/v1.0.0

Scenario: If someone creates a tag like services/api/v1.0.0 (not under apps/), the prefix stripping is a no-op and produces ev-node-services-api, which is actually correct behavior. ✅

However, consider adding validation to ensure the tag starts with apps/:

# After line 46, before directory check
if [[ ! "$APP_PATH" =~ ^apps/ ]]; then
  echo "::error::Tag must be under apps/ directory (got: $APP_PATH)"
  exit 1
fi

This would provide clearer error messages and enforce the intended convention. Add validation →

✅ Testing & Validation

The change is low-risk because:

  • The logic is isolated to image name generation
  • It doesn't affect tag parsing, version extraction, or directory validation
  • The transformation is deterministic and testable
  • Existing validation (directory existence, Dockerfile presence) remains intact

Recommended testing:

  • Verify image names for tags like apps/testapp/v0.4.0ev-node-testapp
  • Verify nested paths like apps/evm/single/v0.2.0ev-node-evm-single
  • Check that old image names are properly deprecated/migrated in registries

📝 Documentation

Minor: The PR description is empty. Consider adding:

## Overview

Strips the redundant `apps/` prefix from Docker image names to improve clarity.

**Before**: `ghcr.io/evstack/ev-node-apps-testapp:v0.4.0`  
**After**: `ghcr.io/evstack/ev-node-testapp:v0.4.0`

This aligns with cleaner naming conventions since the `ev-node-` prefix already indicates the project.

🎯 Impact Assessment

Breaking change: ⚠️ Yes, for consumers pulling images by name

  • Old images: ghcr.io/evstack/ev-node-apps-*
  • New images: ghcr.io/evstack/ev-node-*

Migration considerations:

  1. Are there existing deployments referencing the old image names?
  2. Should old tags be redirected or documented as deprecated?
  3. Does CI/CD in other repos need updating?

Consider adding a note in the release notes or updating deployment docs.


Verdict

Approved ✅ — The change is technically sound, improves naming clarity, and follows bash best practices.

Recommendation: Add the apps/ prefix validation suggested above to make the workflow more defensive and provide clearer error messages if misused.

@julienrbrt julienrbrt requested a review from auricom January 28, 2026 13:11
@codecov
Copy link

codecov bot commented Jan 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.55%. Comparing base (192c3a3) to head (d93278f).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3028      +/-   ##
==========================================
- Coverage   55.61%   55.55%   -0.07%     
==========================================
  Files         116      116              
  Lines       11477    11477              
==========================================
- Hits         6383     6376       -7     
- Misses       4394     4401       +7     
  Partials      700      700              
Flag Coverage Δ
combined 55.55% <ø> (-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 added this pull request to the merge queue Jan 28, 2026
github-merge-queue bot pushed a commit that referenced this pull request Jan 28, 2026
@julienrbrt julienrbrt removed this pull request from the merge queue due to a manual request Jan 28, 2026
@julienrbrt julienrbrt merged commit 4252154 into main Jan 28, 2026
30 checks passed
@julienrbrt julienrbrt deleted the julien/ci-2 branch January 28, 2026 13:39
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