Skip to content
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

Unexpected failure in GitHub Action run #581

Open
spier opened this issue Aug 29, 2023 · 10 comments · Fixed by #608
Open

Unexpected failure in GitHub Action run #581

spier opened this issue Aug 29, 2023 · 10 comments · Fixed by #608
Labels
Type - Maintenance / Cleanup Maintaining / cleaning the repo is the main focus of this issue / PR

Comments

@spier
Copy link
Member

spier commented Aug 29, 2023

In a recent pull request (#579), one of our GitHub Action (GHA) workflow runs failed.

Overview of failed GHAs:
Screenshot 2023-08-29 at 09 49 14

Now when looking at the details of that workflow run, I noticed something interesting:

  • when you click on "workflow file", it takes you to the book.yml file from this branch/PR (which comes from a fork)
  • however the file that really got executed is actually the one on the main branch of the upstream. I am so sure about this as the main branch contains the config for a matrix build for this GHA, while the branch from the PR does not have that matrix build yet)

So at the very least I am starting to wonder how GHAs really work. Seems like I might have some misunderstandings here.

My questions are:

  • which GHAs are executed? the ones on the main branch, or the ones on the PR branch, or both?
  • main and PR contain different versions of the same GHA. what happens in that case?

And lastly:
Why does this GHA run fail at all?

It seems like it is looking for a git ref hive-mind-pattern-1 and cannot find it? Possibly because that ref only exists on the fork but not on the upstream?

Do we have to configure the actions/checkout differently, so that it can find that git ref?
Right now we use ref: ${{ github.head_ref }}, which is defined as:

The head_ref or source branch of the pull request in a workflow run. This property is only available when the event that triggers a workflow run is either pull_request or pull_request_target.

I am suspecting that we are checking out the upstream repo InnerSourceCommons/InnerSourcePatterns but then trying to point to a ref that does not exist on that upsream.

Just not sure how one would configure this correctly?

@spier spier added the Type - Maintenance / Cleanup Maintaining / cleaning the repo is the main focus of this issue / PR label Aug 29, 2023
@spier
Copy link
Member Author

spier commented Aug 29, 2023

@zkoppert and @yuhattor you are my best resources on GitHub Actions :) Maybe you have a hunch what might be going on here? Would be great to get your help.

@zkoppert
Copy link
Member

zkoppert commented Sep 5, 2023

👋🏻 Yeah this is a confusing topic 😓

For the question, "which GHAs are executed? the ones on the main branch, or the ones on the PR branch, or both?"

  • For security purposes, GitHub actions are executed on the fork context, so that means whatever version of the workflow files that are on the fork are what get run. This is less than ideal in this situation, because we likely want to run the workflow files that are on the main branch of the upstream repo.

Given your other points, I think the way forward is to switch to running GitHub actions in the context of the upstream repo when a PR from a fork comes in. This can be done by use the actions trigger pull_request_taarget. The docs for that are here: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target. The important risk to note here is:

Warning: For workflows that are triggered by the pull_request_target event, the GITHUB_TOKEN is granted read/write repository permission unless the permissions key is specified and the workflow can access secrets, even when it is triggered from a fork. Although the workflow runs in the context of the base of the pull request, you should make sure that you do not check out, build, or run untrusted code from the pull request with this event. Additionally, any caches share the same scope as the base branch. To help prevent cache poisoning, you should not save the cache if there is a possibility that the cache contents were altered. For more information, see "Keeping your GitHub Actions and workflows secure: Preventing pwn requests" on the GitHub Security Lab website.

Does that make sense? Happy to help with a pull request to get this changed if you would like.

@spier
Copy link
Member Author

spier commented Oct 31, 2023

I forgot about this issue :)

Saw a different issue with the same GitHub Action today, which is sort of a collateral of this:

  • the GHA did not run correctly on the PRs (from forks)
  • I opted to still merge those PRs (even with these GHA runs failing), as I assumed that things would fix themself when run on main
  • after merging to main, the workflow rain again, making changes to the toc.md file
  • when trying to commit those changes on the main branch (without a PR), we see the issue below (as we have a branch protection rule in place)

remote: error: GH006: Protected branch update failed for refs/heads/main.
remote: error: Changes must be made through a pull request.

This is confusing. So yes, I think it is time to fix this issue :)

@zkoppert sorry for all the notifications today. If you have time to help me configure the pull_request_target event on the book.yml that would be awesome!

@zkoppert
Copy link
Member

@spier No problem. I've opened up #608 to get us going and laid out what I believe are the next steps.

spier pushed a commit that referenced this issue Oct 31, 2023
@spier spier reopened this Oct 31, 2023
@zkoppert
Copy link
Member

I think the other piece of this is specifying ref: ${{ github.head_ref }} during checkout. The net experiment would be to remove that and see if it gets past the checkout phase (it should) but then it will also need to be able to get past the commit stage. There we might need to specify the branch name.

@zkoppert
Copy link
Member

I think that may have actually worked! Looks like checks are passing here: https://github.com/InnerSourceCommons/InnerSourcePatterns/pull/579/checks

@spier
Copy link
Member Author

spier commented Oct 31, 2023

I think the other piece of this is specifying ref: ${{ github.head_ref }} during checkout. The net experiment would be to remove that and see if it gets past the checkout phase (it should) but then it will also need to be able to get past the commit stage. There we might need to specify the branch name.

Well we (and with that I mean me) are certainly learning something about git and GitHub Actions here :)

You are right, the action ran successfully on the PR/branch that I am testing on.

Where previously the action failed on this:
/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin +refs/heads/hive-mind-pattern-1*:refs/remotes/origin/hive-mind-pattern-1* +refs/tags/hive-mind-pattern-1*:refs/tags/hive-mind-pattern-1*

It is now passing:
/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin +f570a5af72f4f1c2c9914283cebfb821de3d9993:refs/remotes/origin/main

f570a5a is the ref of the latest commit on `main. So I guess this is the expected behavior?

Now that leads us to another issue though:
The book.yml action is generating a new table of contents for the book, based on the files on the branch from the PR. i.e. if checkout is now getting the content from main rather than from the PR branch, I guess this won't work?

Will keep looking ... but rather tomorrow. Thanks for all your help so far already Zack!

@spier
Copy link
Member Author

spier commented Oct 31, 2023

I did run a test to confirm that the current configuration of the GHA really checks out main, and not the PR branch.
Result: confirmed :)

Also from the pull_request_target docs:

Avoid using this event if you need to build or run code from the pull request.

So I guess that leaves us in a pickle, or whatever the English idiom is :)

@zkoppert
Copy link
Member

zkoppert commented Nov 1, 2023

Maybe there is a way to break up this action into 2. The first part makes sure that everything builds on the branch so that you know the PR is good to merge, and then a second action detects that a merge to main has occurred and it goes and builds and checks in the files needed on the main branch.

@spier
Copy link
Member Author

spier commented Nov 2, 2023

Yeah, possibly.

There are two separate issues it seems:

  • making a commit via a bot on a protected branch (main) outside of a PR
  • running the workflow on a PR from a fork (need to understand first why this was actually failing)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type - Maintenance / Cleanup Maintaining / cleaning the repo is the main focus of this issue / PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants