-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Don't use codspeed or depot runners in CI jobs on forks #20894
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
.github/workflows/ci.yaml
Outdated
| runs-on: depot-ubuntu-22.04-16 | ||
| needs: determine_changes | ||
| if: ${{ !contains(github.event.pull_request.labels.*.name, 'no-test') && (needs.determine_changes.outputs.code == 'true' || github.ref == 'refs/heads/main') }} | ||
| if: ${{ github.repository == 'astral-sh/ruff' && !contains(github.event.pull_request.labels.*.name, 'no-test') && (needs.determine_changes.outputs.code == 'true' || github.ref == 'refs/heads/main') }} |
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.
I think I slightly prefer github.event.pull_request.head.repo.fork == false as it's more explicit.
An alternative is to disable all CI jobs for forks by adding the same condition at the very top of the file
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.
An alternative is to disable all CI jobs for forks by adding the same condition at the very top of the file
hmm, where exactly would I put this condition at the top of the file? I'd love to do that if it's possible, but I didn't think it was
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.
I think I slightly prefer
github.event.pull_request.head.repo.fork == falseas it's more explicit.
I don't think this would fix the issue I'm trying to fix, because the specific issue I'm trying to fix is that these jobs are being run on pushes to the main branch of the AlexWaygood/ruff repo. I.e., that would be a push event rather than a pull_request event.
But I'd also like these jobs that can't be run on forks disabled if I make a pull request against my own fork (which I don't do that often, but have done in the past, for testing purposes!) 😄
The expression I'm using here is basically identical to the example GitHub has in its docs for if conditions, FWIW: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#example-only-run-job-for-specific-repository. And it's also what we already use elsewhere in this workflow file:
ruff/.github/workflows/ci.yaml
Line 1015 in 4b7f184
| if: ${{ github.repository == 'astral-sh/ruff' && !contains(github.event.pull_request.labels.*.name, 'no-test') && (needs.determine_changes.outputs.ty == 'true' || github.ref == 'refs/heads/main') }} |
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.
Well...
I guess the ideal would be to use a different runner based on whether this is a fork or not. But I don't know how easily this is possible (and it won't work with codspeed).
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.
I guess the ideal would be to use a different runner based on whether this is a fork or not. But I don't know how easily this is possible (and it won't work with codspeed).
With Claude's help, I think I figured this out! (Except for Codspeed, of course)
|
.github/workflows/ci.yaml
Outdated
| runs-on: depot-ubuntu-22.04-16 | ||
| needs: determine_changes | ||
| if: ${{ !contains(github.event.pull_request.labels.*.name, 'no-test') && (needs.determine_changes.outputs.code == 'true' || github.ref == 'refs/heads/main') }} | ||
| if: ${{ github.repository == 'astral-sh/ruff' && !contains(github.event.pull_request.labels.*.name, 'no-test') && (needs.determine_changes.outputs.code == 'true' || github.ref == 'refs/heads/main') }} |
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.
Well...
I guess the ideal would be to use a different runner based on whether this is a fork or not. But I don't know how easily this is possible (and it won't work with codspeed).
1d0f18a to
4a8ad32
Compare
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
…rable * origin/main: Don't use codspeed or depot runners in CI jobs on forks (#20894) [ty] cache Type::is_redundant_with (#20477) Fix run-away for mutually referential instance attributes (#20645) [ty] Limit shown import paths to at most 5 unless ty runs with `-v` (#20912) [ty] Use field-specifier return type as the default type for the field (#20915) [ty] Do not assume that `field`s have a default value (#20914) [ty] Fix match pattern value narrowing to use equality semantics (#20882) Update setup instructions for Zed 0.208.0+ (#20902) Move TOML indent size config (#20905) [syntax-errors]: implement F702 as semantic syntax error (#20869) [ty] Heterogeneous unpacking support for unions (#20377) [ty] refactor `Place` (#20871) Auto-accept snapshot changes as part of typeshed-sync PRs (#20892) [`airflow`] Add warning to `airflow.datasets.DatasetEvent` usage (`AIR301`) (#20551) [`flake8-pyi`] Fix operator precedence by adding parentheses when needed (`PYI061`) (#20508) [`pyupgrade`] Fix false negative for `TypeVar` with default argument in `non-pep695-generic-class` (`UP046`) (#20660) Update parser snapshots (#20893) Fix syntax error false positives for escapes and quotes in f-strings (#20867)
…nt-sets * dcreager/non-non-inferable: Don't use codspeed or depot runners in CI jobs on forks (#20894) [ty] cache Type::is_redundant_with (#20477) Fix run-away for mutually referential instance attributes (#20645) [ty] Limit shown import paths to at most 5 unless ty runs with `-v` (#20912) [ty] Use field-specifier return type as the default type for the field (#20915) [ty] Do not assume that `field`s have a default value (#20914) [ty] Fix match pattern value narrowing to use equality semantics (#20882) Update setup instructions for Zed 0.208.0+ (#20902) Move TOML indent size config (#20905) [syntax-errors]: implement F702 as semantic syntax error (#20869) [ty] Heterogeneous unpacking support for unions (#20377) [ty] refactor `Place` (#20871) Auto-accept snapshot changes as part of typeshed-sync PRs (#20892) [`airflow`] Add warning to `airflow.datasets.DatasetEvent` usage (`AIR301`) (#20551) [`flake8-pyi`] Fix operator precedence by adding parentheses when needed (`PYI061`) (#20508) [`pyupgrade`] Fix false negative for `TypeVar` with default argument in `non-pep695-generic-class` (`UP046`) (#20660) Update parser snapshots (#20893) Fix syntax error false positives for escapes and quotes in f-strings (#20867)
Summary
Currently I get a GitHub notification every time I sync my Ruff fork with
astral-sh/ruff. The notification informs me that theciworkflow failed, because it tried to run various jobs on Depot or codspeed runners that my fork doesn't have access to. This is kinda annoying, and -- while I don't really need to sync my fork on regular basis, since I have the ability to create branches directly on the upstream repo -- I'm guessing this also occurs for all our third-party contributors who have forks of Ruff.The solution to this is not to run CI jobs on forks if they require a Depot or Codspeed runner. Unfortunately there doesn't appear to be a way to say "run this workflow on all pushes to
main, but skip the whole workflow if it's a fork" -- you have to include theif github.repository == 'astral-sh/ruff'check in each separate CI job inside the workflow.Another solution would be to say that contributors who have forks should simply disable GitHub Actions altogether on their forks. But running CI from a fork can be pretty useful, especially if you're a third-party contributor, as a way to test changes without opening a PR upstream (which can result in notifications for the upstream maintainers, even if the PR is opened in draft mode).
Test Plan
CI on this PR