Skip to content

Conversation

@auricom
Copy link
Contributor

@auricom auricom commented Nov 3, 2025

Now evm-single docker images tags should be created for github tags sequencer/single/vx.x.x

@claude
Copy link
Contributor

claude bot commented Nov 3, 2025

Claude finished @auricom's task —— View job


Comprehensive Code Review

I've reviewed the workflow changes in this PR. Here's my detailed analysis:


Summary

This PR modifies GitHub Actions workflows to implement selective Docker image building for ev-node-evm-single. The changes allow the workflow to:

  1. Build images for tags matching sequencers/single/* pattern (stripping the prefix)
  2. Continue building for PR tags (pr-*)
  3. Skip building for other events (like main branch pushes)

Additionally, the PR broadens the tag trigger in ci_release.yml from v* to * to capture all tag patterns.


Code Quality & Architecture

✅ Strengths:

  • Clean separation of concerns with the check-tag step extracting tag logic
  • Proper use of workflow conditionals with if: steps.check-tag.outputs.should-push == 'true'
  • Consistent pattern matching using bash parameter expansion

⚠️ Concerns:

  1. Pattern Mismatch (Critical): .github/workflows/test.yml:66

    if [[ "$TAG" == sequencers/single/* ]]; then

    The PR description states tags should match sequencer/single/vx.x.x (singular), but the code checks for sequencers/single/* (plural). This is a critical discrepancy that will cause the workflow to fail its intended purpose.

    Recommendation: Clarify which pattern is correct and update either the code or the PR description. If it should be singular, change line 66 to:

    if [[ "$TAG" == sequencer/single/* ]]; then
  2. Missing Workflow Context: The changes to ci_release.yml trigger on all tags (*), but there's no clear documentation about what other tag patterns exist or are expected beyond sequencer/single/* and v*. This could lead to confusion.

  3. Test Coverage Gap: The docker-upgrade-tests job (line 136-150 in test.yml) uses ${{ inputs.image-tag }} directly without transformation, but the image may now be tagged with a stripped version. This will fail when triggered by sequencer/single/* tags.

    env:
      EVM_SINGLE_NODE_IMAGE_TAG: ${{ inputs.image-tag }}

    Should likely be:

    env:
      EVM_SINGLE_NODE_IMAGE_TAG: ${{ steps.check-tag.outputs.docker-tag }}

    However, this requires the output to be available across jobs, which it currently isn't.


Error Handling & Resilience

⚠️ Issues:

  1. Silent Failure Mode: When should-push=false, the workflow step is skipped silently. While this may be intentional, it makes debugging harder. Consider adding a notification step or clearer logging.

  2. Missing Dependency Declaration: The docker-upgrade-tests job depends on build-ev-node-evm-single-image, but if that job skips the build (when should-push=false), the upgrade tests will fail trying to pull a non-existent image.

    Recommendation: Add conditional logic to docker-upgrade-tests:

    docker-upgrade-tests:
      needs: build-ev-node-evm-single-image
      if: needs.build-ev-node-evm-single-image.result == 'success'
  3. No Validation of Tag Format: The script doesn't validate that stripped tags are valid Docker tag names. While unlikely to be an issue with GitHub tags, edge cases could cause problems.


Security

✅ Strengths:

  • Proper use of ${{ secrets.GITHUB_TOKEN }} for authentication
  • No hard-coded credentials
  • Permissions are appropriately scoped (packages: write)

⚠️ Minor Concerns:

  1. Tag Injection Risk (Low): While GitHub-provided inputs.image-tag is generally safe, the bash script doesn't validate or sanitize the input before using it in shell commands. An attacker with write access could potentially craft malicious tag names.

    Recommendation: Add input validation:

    TAG="${{ inputs.image-tag }}"
    # Validate tag format (alphanumeric, hyphens, underscores, slashes, dots only)
    if [[ ! "$TAG" =~ ^[a-zA-Z0-9._/-]+$ ]]; then
      echo "Invalid tag format" >&2
      exit 1
    fi

Performance & Resource Efficiency

✅ Positive:

  • The selective building approach reduces unnecessary Docker builds, saving CI time and resources
  • Multi-platform builds (linux/amd64,linux/arm64) are maintained

⚠️ Consideration:

  • Building is skipped for most events, which is efficient, but ensure this aligns with your deployment strategy. Main branch pushes won't build ev-node-evm-single images anymore.

Testing & Reliability

❌ Critical Issue:

The docker-upgrade-tests job will fail with the current implementation:

# Line 150 in test.yml
env:
  EVM_SINGLE_NODE_IMAGE_TAG: ${{ inputs.image-tag }}

When a tag like sequencer/single/v1.0.0 is pushed:

  • The image is built with tag v1.0.0 (prefix stripped)
  • But the test tries to pull using tag sequencer/single/v1.0.0 (original)
  • This mismatch will cause the test to fail

Recommended Fix:
The check-tag step's outputs need to be shared across jobs. Options:

  1. Move the tag determination logic to a separate job that outputs the result
  2. Duplicate the logic in the test job
  3. Pass the transformed tag through the workflow call interface

Example solution (option 1):

jobs:
  determine-tags:
    runs-on: ubuntu-latest
    outputs:
      evm-single-tag: ${{ steps.check.outputs.docker-tag }}
      should-build: ${{ steps.check.outputs.should-push }}
    steps:
      - id: check
        run: |
          # Same logic as check-tag step
          
  build-ev-node-evm-single-image:
    needs: determine-tags
    if: needs.determine-tags.outputs.should-build == 'true'
    # ... rest of job
    
  docker-upgrade-tests:
    needs: [determine-tags, build-ev-node-evm-single-image]
    env:
      EVM_SINGLE_NODE_IMAGE_TAG: ${{ needs.determine-tags.outputs.evm-single-tag }}

Documentation & Developer Experience

⚠️ Gaps:

  1. Inadequate PR Description: The description doesn't explain:

    • Why this change is needed
    • What the previous behavior was
    • What other systems might be affected
    • Examples of tags that will/won't trigger builds
  2. No Inline Comments: The bash script in the workflow would benefit from comments explaining the logic, especially the tag transformation.

  3. Missing CHANGELOG Entry: This is a significant behavioral change that should be documented in a CHANGELOG if one exists.


Additional Observations

  1. Inconsistent Naming: The PR branch is claude/docker_tags_fix and mentions this is a "fix", but there's no reference to what bug is being fixed. This appears to be a feature addition rather than a bug fix.

  2. Related Changes: I notice the diff also includes a new ghcr-prune.yml workflow file, but this wasn't discussed in the PR description. While it's related to Docker image management, it should probably be mentioned.

  3. Missing Edge Cases: What happens if a tag matches both patterns (unlikely but possible)? The current logic would match the first pattern only.


Recommendations Priority

🔴 Critical (Must Fix):

  1. Fix the pattern mismatch: sequencers vs sequencer
  2. Fix the test job to use the correct transformed tag

🟡 High Priority (Should Fix):

  1. Add tag format validation for security
  2. Document the change more thoroughly in the PR description
  3. Add conditional logic to prevent test failures when builds are skipped

🟢 Nice to Have:

  1. Add inline comments to the bash script
  2. Consider adding a notification/logging step when builds are skipped
  3. Update CHANGELOG if applicable

Verdict

The approach is sound and will improve CI efficiency, but there are critical bugs that will prevent this from working as intended. The pattern mismatch and test job issues must be resolved before merging. Once those are addressed, this will be a solid improvement to the CI pipeline.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2025

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

BuildFormatLintBreakingUpdated (UTC)
✅ passed⏩ skipped✅ passed✅ passedNov 4, 2025, 1:54 PM

@auricom auricom force-pushed the claude/docker_tags_fix branch from 34412ca to 339a1ce Compare November 3, 2025 15:43
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @auricom, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces two new Git submodules to the project: forge-std and openzeppelin-contracts. These additions suggest an effort to integrate well-known and trusted libraries for smart contract development directly into the repository, streamlining dependency management and potentially enhancing the project's capabilities for building and interacting with smart contracts.

Highlights

  • Submodule Addition: This PR adds two new submodules: forge-std and openzeppelin-contracts.
  • Dependency Management: The addition of these submodules likely aims to incorporate standard libraries and contracts directly into the project, improving dependency management.
Ignored Files
  • Ignored by pattern: .github/workflows/** (2)
    • .github/workflows/ci_release.yml
    • .github/workflows/test.yml
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds forge-std and openzeppelin-contracts as Git submodules. My primary feedback is that the pull request's title and description, which refer to fixing Docker tags, do not align with the actual changes. This discrepancy is confusing and could hinder future maintenance and understanding of the project's history. Please update the PR title and description to accurately reflect that project dependencies are being added. I have also included a specific comment regarding dependency management for the new submodules.

@codecov
Copy link

codecov bot commented Nov 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.37%. Comparing base (83996f2) to head (9b69452).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2809   +/-   ##
=======================================
  Coverage   62.37%   62.37%           
=======================================
  Files          82       82           
  Lines        7304     7304           
=======================================
  Hits         4556     4556           
  Misses       2203     2203           
  Partials      545      545           
Flag Coverage Δ
combined 62.37% <ø> (ø)

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.

@auricom auricom force-pushed the claude/docker_tags_fix branch from 339a1ce to a4de289 Compare November 3, 2025 15:48
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utaCK

@auricom auricom enabled auto-merge November 4, 2025 13:53
@auricom auricom disabled auto-merge November 4, 2025 13:57
@auricom auricom marked this pull request as draft November 4, 2025 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants