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

Fix workflow runs from forks #905

Merged
merged 2 commits into from
Jan 21, 2025
Merged

Fix workflow runs from forks #905

merged 2 commits into from
Jan 21, 2025

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Jan 8, 2025

As discovered in #904 (comment), it seems we need to bring to main a version of the "Pytest" workflow that doesn't run on all PRs to main and that uses the correct data for the checkout action when being dispatched from the "Receive PR" workflow. This PR cherry-picks the two commits from #904 that achieve this.

How to review

  • Read the diff and note that the CI checks all pass.
  • Note that this PR should trigger the "Pytest" workflow only once, after successfully completing the "Receive PR" workflow.

PR checklist

  • Continuous integration checks all ✅ These fail because of the underlying issue. Merging the changes will address the issue.
  • Add or expand tests; coverage checks both ✅
  • [ ] Add, expand, or update documentation. Just CI.
  • [ ] Update release notes. Just CI.

@glatterf42 glatterf42 added the ci Continuous integration label Jan 8, 2025
@glatterf42 glatterf42 self-assigned this Jan 8, 2025
@glatterf42
Copy link
Member Author

Okay, even though this PR is from within IIASA's repo, the only "Pytest" run that is triggered uses the version currently on main, so it's still failing. I find it curious that these failing tests are not registered here, instead showing the required tests as "expected" still. This makes me wonder if they will update to "successful" once all of them are.

@glatterf42
Copy link
Member Author

From this GitHub blog, it sounds like we might have to save things like coverage and test failure rate (or so) as artifacts and comment them on the PR manually:

The workflow processing the PR should then store any results like code coverage or failed/passed tests in artifacts and exit. The following workflow then starts on workflow_run where it is granted write permission to the target repository and access to repository secrets, so that it can download the artifacts and make any necessary modifications to the repository or interact with third party services that require repository secrets (e.g. API tokens).

This sounds less than ideal. Alternatively, I found this suggestion: use the triggering_actor to decide if the workflow has access to secrets. This way, just anyone (without write access to the main repo) would open the PR and the workflow fails (because it doesn't have access to secrets), but once re-run from someone with the right permissions, the workflow is allowed to access the secrets.
This is still prone to human error, but should be fine in our case, I think. We don't often have time-critical contributions, almost never ones from outside IIASA. So we could double and triple check any suspicious code before re-running a workflow.

@khaeru
Copy link
Member

khaeru commented Jan 8, 2025

it sounds like we might have to save things like coverage and test failure rate (or so) as artifacts and comment them on the PR manually

Urgh, that's definitely too complicated.

Alternatively, I found this suggestion: use the triggering_actor to decide if the workflow has access to secrets.

This seems like a good alternative. If I understand, a maintainer would need to trigger a re-run each time, right? I think that is an acceptable compromise, given that we will not use this so frequently: we can continue to encourage teammates to use branches within the iiasa repo, so it would be only outside collaborators.

@glatterf42
Copy link
Member Author

Not even that: all colleagues from the MESSAGEix Devs team have write access, too. So even PRs from their forks would just run immediately, given their permissions. This would only affect PRs from people not on the team (or otherwise having write access to this repo), if I understand correctly.

@khaeru
Copy link
Member

khaeru commented Jan 8, 2025

Okay, that sounds great. Should we try to implement it via this PR?

@glatterf42
Copy link
Member Author

glatterf42 commented Jan 8, 2025

I tried doing that just now. I think for this to work as intended, the github.event.workflow_run.head_branch needs to point to the fork's branch, and I'm not sure it does that. It could also point to main as the head branch triggering the "Pytest" workflow (seeing that the "Receive" workflow is executed on main), in which case we would only be testing main, which does not seem meaningful. I'm not sure how best to test this (other than merging something to some kind of main, which could also be one of our forks' mains, I guess).

@glatterf42 glatterf42 force-pushed the fix/workflow-runs-from-forks branch from 319620a to 04ed41a Compare January 8, 2025 14:22
@khaeru khaeru force-pushed the fix/workflow-runs-from-forks branch from 04ed41a to 4e7dd0a Compare January 21, 2025 08:56
@khaeru
Copy link
Member

khaeru commented Jan 21, 2025

As discussed, we'll drop the 3rd commit re: triggering_actor here; merge; check the effects; and then continue in a new PR.

@khaeru khaeru force-pushed the fix/workflow-runs-from-forks branch from 4e7dd0a to 5d4b0d0 Compare January 21, 2025 08:58
@khaeru khaeru force-pushed the fix/workflow-runs-from-forks branch from 5d4b0d0 to 24a32f0 Compare January 21, 2025 09:01
@khaeru khaeru merged commit 3b98b92 into main Jan 21, 2025
6 checks passed
@khaeru khaeru deleted the fix/workflow-runs-from-forks branch January 21, 2025 09:04
@khaeru
Copy link
Member

khaeru commented Jan 21, 2025

Okay, after doing this, I rebased both #904 and #808 on main, triggering CI to run. What I see:

  • The "receive" workflow runs and does not fail.
  • "Test" workflow runs are triggered. Per the list, they do not appear associated with the respective PRs.
  • On the two PR pages, the required checks/jobs are all showing "Expected — Waiting for status to be reported". So they are not associated with the jobs in the triggered workflow.
  • The "Test" workflow runs fail in the step "Check out message_ix (workflow_run)", for instance here:
  /usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=100 origin +refs/heads/macro_update*:refs/remotes/origin/macro_update* +refs/tags/macro_update*:refs/tags/macro_update*
  The process '/usr/bin/git' failed with exit code 1
  Waiting 14 seconds before trying again
  /usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=100 origin +refs/heads/macro_update*:refs/remotes/origin/macro_update* +refs/tags/macro_update*:refs/tags/macro_update*
  The process '/usr/bin/git' failed with exit code 1
  Waiting 11 seconds before trying again
  /usr/bin/git -c protocol.version=2 fetch --no-tags --prune --no-recurse-submodules --depth=100 origin +refs/heads/macro_update*:refs/remotes/origin/macro_update* +refs/tags/macro_update*:refs/tags/macro_update*
  Error: The process '/usr/bin/git' failed with exit code 1
  • I guess this is occurring because, e.g. there is no remotes/origin/macro_update in the iiasa/message_ix repo; within that repo, the particular remote (glatterf42/message_ix or daymontas1/message_ix) is not known as 'origin'. This is more or less what @glatterf42 guessed at:

    I think for this to work as intended, the github.event.workflow_run.head_branch needs to point to the fork's branch, and I'm not sure it does that.

I guess this means back to the drawing-board! Will link to this comment from any follow-up PR(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants