Skip to content

Fix incorrect "fork check" condition in validate_pr workflow #2161

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Fix incorrect "fork check" condition in validate_pr workflow #2161

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/validate-pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ on:
jobs:
validate-pr:
name: build
if: github.repository_owner == 'elastic' # this action will fail if executed from a fork
if: github.event.pull_request.head.repo.owner.login == 'elastic' # this action will fail if executed from a fork
Copy link
Contributor

@swallez swallez Jun 28, 2023

Choose a reason for hiding this comment

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

I'm not familiar with the github object model. This test is because the action checks out elastic/clients-flight-recorder which is a private repo. So the condition should test that the user running the action is a member of the elastic org. I don't think this is what this new expression will test?

Copy link
Member Author

@flobernd flobernd Jun 28, 2023

Choose a reason for hiding this comment

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

Correct. If the workflow is triggered from a fork, the secrets context is not available and in consequence no token/PAT is supplied to the checkout action which ultimately causes the workflow to fail.

The condition seems to be intended to work around that problem by disabling the workflow if it was triggered from a fork.

The expression used in the original condition however does not work for that purpose, because github.repository_owner always evaluates to the BASE/target repository (elastic/elasticsearch-specification in this case). This for example happened in #2160.

To check the HEAD/source branch instead, we have to use github.event.pull_request.head.repo.owner.login. This expression would evaluate to e.g. flobernd if I trigger this workflow from a fork (e.g. flobernd/elasticsearch-specification).

Copy link
Member Author

@flobernd flobernd Jun 28, 2023

Choose a reason for hiding this comment

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

Probably this condition is still not the perfect one to use, because even a fork from inside the elastic organization (e.g. elastic/elasticsearch-specification-clone) might not have access to the secrets context. However there might be a special rule for "inside organization" forks that I don't remember right now.

runs-on: ubuntu-latest
env:
STACK_VERSION: 8.9-SNAPSHOT
Expand Down