Skip to content
This repository was archived by the owner on Jan 16, 2026. It is now read-only.

Comments

fix(node/ci): fixes several CI issues#2807

Merged
theochap merged 4 commits intomainfrom
theo/fix-op-deployer-kurtosis
Sep 3, 2025
Merged

fix(node/ci): fixes several CI issues#2807
theochap merged 4 commits intomainfrom
theo/fix-op-deployer-kurtosis

Conversation

@theochap
Copy link
Member

@theochap theochap commented Sep 2, 2025

Description

Fixes CI by:

Copilot AI review requested due to automatic review settings September 2, 2025 16:17
@theochap theochap self-assigned this Sep 2, 2025
@theochap theochap added M-tests Meta: Testing related A-node Area: cl node (eq. Go op-node) handles single-chain consensus labels Sep 2, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Updates the optimism-package dependency to a specific commit hash and removes the optimism package configuration file to fix Kurtosis CI issues.

  • Updates optimism-package reference from commit 219d5f9e to commit 998796c0 in justfile
  • Removes the optimism_package.json configuration file

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
tests/justfile Updates the DEFAULT_OP_PACKAGE_PATH to use a newer commit hash
tests/.config/optimism_package.json Removes the entire configuration file

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@claude
Copy link
Contributor

claude bot commented Sep 2, 2025

Claude finished @theochap's task —— View job


Pull Request Review ✅

I've completed my review of this PR which updates the optimism-package dependency to fix Kurtosis CI issues.

Progress:

  • Read and analyze the changed files to understand the modifications
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Assess security concerns
  • Review test coverage implications
  • Provide comprehensive review feedback

Review Feedback

Changes Summary

This PR makes two simple but effective changes:

  1. Updates optimism-package commit hash in tests/justfile:3 from 219d5f9e104951d0ef6ed9586f9f6baa0ecbd980 to 998796c0f3bb478d63d729e65f0b76e24112e00d
  2. Removes the configuration file tests/.config/optimism_package.json that only contained repository and branch info

✅ Code Quality & Best Practices

  • Clean implementation: The changes are minimal and focused on the specific issue
  • Follows existing patterns: Uses the established DEFAULT_OP_PACKAGE_PATH variable pattern
  • Good justification: The PR description clearly states the intent to fix Kurtosis CI issues

✅ Potential Bugs/Issues

  • No bugs identified: The changes are straightforward dependency updates
  • Proper Git reference format: The commit hash format is correct for GitHub package references
  • Fallback logic preserved: The justfile's conditional logic for OP_PACKAGE_PATH handling remains intact

✅ Performance Considerations

  • Neutral performance impact: Dependency version updates typically don't affect runtime performance
  • Potentially improved CI reliability: Newer package versions may have bug fixes that improve CI stability

✅ Security Concerns

  • Low security risk: This is a testing dependency update, not production code
  • Commit hash pinning: Using specific commit hashes (vs branches/tags) is a security best practice as it prevents supply chain attacks from updated dependencies
  • Trusted source: ethpandaops/optimism-package is a legitimate Ethereum testing framework

✅ Test Coverage Implications

  • Config file removal rationale: The removed optimism_package.json only contained basic repository info (repository: "https://github.com/ethpandaops/optimism-package", branch: "main") that's now redundant with the explicit commit hash reference
  • Test framework intact: All devnet YAML configurations in tests/devnets/ remain unchanged, so test coverage should be preserved
  • CI integration maintained: The justfile logic for Kurtosis integration remains functional

Recommendations

  1. Consider adding a comment in the justfile explaining why this specific commit hash was chosen (e.g., "# Updated to fix Kurtosis CI issues - see PR fix(node/ci): fixes several CI issues #2807")
  2. Monitor CI results to ensure the update resolves the intended Kurtosis issues
  3. Future maintenance: Consider establishing a process for regularly updating this dependency to avoid similar CI issues

Overall Assessment

This is a well-executed maintenance PR that addresses a specific CI issue with minimal, targeted changes. The approach of removing redundant configuration and updating to a specific commit hash is sound. The changes are low-risk and should effectively resolve the stated Kurtosis CI problems.

Recommendation: ✅ Approve


@codecov
Copy link

codecov bot commented Sep 2, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.0%. Comparing base (5228120) to head (77100bd).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

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

@theochap theochap force-pushed the theo/fix-op-deployer-kurtosis branch 3 times, most recently from 65210e2 to 386513b Compare September 3, 2025 08:02
@theochap theochap force-pushed the theo/fix-op-deployer-kurtosis branch from 386513b to 04a4365 Compare September 3, 2025 08:37
@theochap theochap force-pushed the theo/fix-op-deployer-kurtosis branch from 9411c0c to 792e37a Compare September 3, 2025 09:54
@theochap theochap changed the title fix(node/test): fix op-deployer config fix(node/ci): fixes several CI issues Sep 3, 2025
@theochap theochap enabled auto-merge September 3, 2025 10:16
@theochap theochap added this pull request to the merge queue Sep 3, 2025
Merged via the queue into main with commit b68d533 Sep 3, 2025
28 of 35 checks passed
@theochap theochap deleted the theo/fix-op-deployer-kurtosis branch September 3, 2025 11:19
theochap added a commit to ethereum-optimism/optimism that referenced this pull request Dec 10, 2025
## Description

Fixes CI by:
- updating all dependencies (optimism-package, kurtosis, monorepo)
- fixing cargo hack failures caused by op-rs/kona#2762 
- fixing udeps failure caused by op-rs/kona#2762 
- fixing sysgo job failure upstream caused by op-rs/kona#2762
theochap added a commit to ethereum-optimism/optimism that referenced this pull request Jan 14, 2026
## Description

Fixes CI by:
- updating all dependencies (optimism-package, kurtosis, monorepo)
- fixing cargo hack failures caused by #2762 
- fixing udeps failure caused by #2762 
- fixing sysgo job failure upstream caused by #2762
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A-node Area: cl node (eq. Go op-node) handles single-chain consensus M-tests Meta: Testing related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants