Skip to content

Conversation

@gianbelinche
Copy link
Contributor

@gianbelinche gianbelinche commented Oct 31, 2025

Motivation
The L2 CI is not running on some PRs it should.

Description
The problem is that the required checks Integration Test and Lint have the same name for both L1 and L2. So if one of them passes, then the other is not needed to merge. This can be a problem in the case that the L1 CI finishes before the L2 one starts, in that case the L2 CI will not run.

The solution is the following:

  • Add Integration Test L2 and Lint L2 to the branch protection rules
  • Rename the corresponding jobs on the L2 workflow.

With only those changes, we will always require the L2 workflows to run in order to merge, since we want to be able only run the L1 CI in PRs that don't touch the L2, we need a way to skip it.

Note: The current way of not running a workflow doesn't work, since skipping a complete workflow with paths: makes the jobs abscent, what we need is for the jobs to be marked as skipped.

To do this, dorny/paths-filter@v3 action is used, which performs the same action as the paths: option, but as a job itself.
Then with the result of this job we can add ifs to the rest of the jobs to mark if they should be run.

Examples with a fork of ethrex with the branch protection already active (For Integration Test L2)

You can also check that when the PRs were merged to main, all checks ran even though the PRs skipped the checks.
https://github.com/gianbelinche/ethrex/commits/main/

Copilot AI review requested due to automatic review settings October 31, 2025 17:44
@gianbelinche gianbelinche requested a review from a team as a code owner October 31, 2025 17:44
@github-actions github-actions bot added L1 Ethereum client L2 Rollup client labels Oct 31, 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

This PR refactors the GitHub Actions workflows for L1 and L2 to use dynamic path-based filtering instead of static paths and paths-ignore configurations. The changes introduce a detect-changes job that determines whether tests should run based on file changes, improving workflow efficiency by conditionally executing jobs.

  • Replaced static paths/paths-ignore filters with a dynamic detect-changes job using dorny/paths-filter@v3
  • Added conditional execution to all jobs based on the detect-changes output
  • Renamed required checks to "Lint L2" and "Integration Test L2" for the L2 workflow

Reviewed Changes

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

File Description
.github/workflows/pr-main_l2.yaml Introduces detect-changes job with path filtering for L2-specific paths; adds conditional execution to all jobs based on detected changes
.github/workflows/pr-main_l1.yaml Introduces detect-changes job with negated path filter to exclude L2 changes; adds conditional execution to all jobs based on detected changes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- name: finish
id: finish
run: |
if [[ "${GITHUB_EVENT_NAME}" != "pull_request" ]]; then
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

The environment variable should be $GITHUB_EVENT_NAME instead of ${GITHUB_EVENT_NAME}. While both syntaxes work in bash, GitHub Actions sets this as GITHUB_EVENT_NAME and the standard pattern in GitHub Actions is to use the $GITHUB_EVENT_NAME syntax for consistency with GitHub Actions documentation.

Suggested change
if [[ "${GITHUB_EVENT_NAME}" != "pull_request" ]]; then
if [[ "$GITHUB_EVENT_NAME" != "pull_request" ]]; then

Copilot uses AI. Check for mistakes.
- name: finish
id: finish
run: |
if [[ "${GITHUB_EVENT_NAME}" != "pull_request" ]]; then
Copy link

Copilot AI Oct 31, 2025

Choose a reason for hiding this comment

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

The environment variable should be $GITHUB_EVENT_NAME instead of ${GITHUB_EVENT_NAME}. While both syntaxes work in bash, GitHub Actions sets this as GITHUB_EVENT_NAME and the standard pattern in GitHub Actions is to use the $GITHUB_EVENT_NAME syntax for consistency with GitHub Actions documentation.

Suggested change
if [[ "${GITHUB_EVENT_NAME}" != "pull_request" ]]; then
if [[ "$GITHUB_EVENT_NAME" != "pull_request" ]]; then

Copilot uses AI. Check for mistakes.
@github-project-automation github-project-automation bot moved this to In Review in ethrex_l1 Oct 31, 2025
@ilitteri ilitteri added this pull request to the merge queue Oct 31, 2025
Merged via the queue into main with commit db22fbc Oct 31, 2025
60 of 62 checks passed
@ilitteri ilitteri deleted the make-l2-ci-required branch October 31, 2025 21:50
@github-project-automation github-project-automation bot moved this from In Review to Done in ethrex_l1 Oct 31, 2025
xqft pushed a commit that referenced this pull request Nov 11, 2025
**Motivation**
The L2 CI is not running on some PRs it should.
<!-- Why does this pull request exist? What are its goals? -->

**Description**
The problem is that the required checks `Integration Test` and `Lint`
have the same name for both L1 and L2. So if one of them passes, then
the other is not needed to merge. This can be a problem in the case that
the L1 CI finishes before the L2 one starts, in that case the L2 CI will
not run.

The solution is the following:
- Add `Integration Test L2` and `Lint L2` to the branch protection rules
- Rename the corresponding jobs on the L2 workflow.

With only those changes, we will always require the L2 workflows to run
in order to merge, since we want to be able only run the L1 CI in PRs
that don't touch the L2, we need a way to skip it.

Note: The current way of not running a workflow doesn't work, since
skipping a complete workflow with `paths:` makes the jobs abscent, what
we need is for the jobs to be marked as skipped.

To do this,
[dorny/paths-filter@v3](https://github.com/dorny/paths-filter) action is
used, which performs the same action as the `paths:` option, but as a
job itself.
Then with the result of this job we can add ifs to the rest of the jobs
to mark if they should be run.

Examples with a fork of ethrex with the branch protection already active
(For Integration Test L2)
- [PR with changes only in
L1](gianbelinche#4): It ran L1 checks and
skipped L2 ones.
- [PR with changes only in
L2](gianbelinche#3): It ran L2 checks and
skipped L1 ones.
- [PR with both L1 and L2
changes](gianbelinche#5): It ran both L1
and L2 checks.

You can also check that when the PRs were merged to main, all checks ran
even though the PRs skipped the checks.
https://github.com/gianbelinche/ethrex/commits/main/

<!-- A clear and concise general description of the changes this PR
introduces -->

<!-- Link to issues: Resolves #111, Resolves #222 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L1 Ethereum client L2 Rollup client

Projects

Status: Done
Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants