Fail explicitly with pull requests opened from forks #2580
Conversation
This reverts commit e617f37.
There was a problem hiding this comment.
LGTM!
In the future, we could use the pull_request_target trigger instead.
This requires a little bit of extra work as we would have to check-out the user branch, copy only the specification (*.ts) files to the base branch and run the checks from here.
PRs that change the compiler itself can not be checked this way for security reasons, but it should be really really rare for an outside collaborator to change the compiler code.
.github/workflows/validate-pr.yml
Outdated
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Check pull request was opened from branch | ||
| if: ${{ github.event.pull_request.head.repo.full_name != 'elastic/elasticsearch-specification' }} |
There was a problem hiding this comment.
I think you can omit the template syntax here. Just the condition without ${{}} should be fine.
There was a problem hiding this comment.
Thanks, done in fea078b
This reverts commit 14bcc30.
|
I confirmed in #2582 that we now get a failure with the right message. And that from branches, validations does run. Merging.
|
This is a common problem, see #2555 and #2577 for recent examples. But not that common, and I tend to forget about it after a few months. Indeed, the current condition does not actually skip validation, and we don't want that anyway: we want to fail when we should have run validation but could not do it. And explain why we failed.
If the pull request was from an Elastic employee (95% of cases), then they can reopen it from a branch. If it was community-contributed, we need to do it for them (without closing the original pull request, thus preserving attribution when merging).
From a branch, the build was correctly triggered: https://github.com/elastic/elasticsearch-specification/actions/runs/9250931808/job/25445405796?pr=2580 (and failed as expected due to the extra file I have now removed).