-
Notifications
You must be signed in to change notification settings - Fork 124
fix(l1,l2): make l2 integration tests and lint required #5142
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this 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-ignorefilters with a dynamicdetect-changesjob usingdorny/paths-filter@v3 - Added conditional execution to all jobs based on the
detect-changesoutput - 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 |
Copilot
AI
Oct 31, 2025
There was a problem hiding this comment.
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.
| if [[ "${GITHUB_EVENT_NAME}" != "pull_request" ]]; then | |
| if [[ "$GITHUB_EVENT_NAME" != "pull_request" ]]; then |
| - name: finish | ||
| id: finish | ||
| run: | | ||
| if [[ "${GITHUB_EVENT_NAME}" != "pull_request" ]]; then |
Copilot
AI
Oct 31, 2025
There was a problem hiding this comment.
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.
| if [[ "${GITHUB_EVENT_NAME}" != "pull_request" ]]; then | |
| if [[ "$GITHUB_EVENT_NAME" != "pull_request" ]]; then |
**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 -->
Motivation
The L2 CI is not running on some PRs it should.
Description
The problem is that the required checks
Integration TestandLinthave 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:
Integration Test L2andLint L2to the branch protection rulesWith 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/