set PRs and their dirty state as output #17
Conversation
sources/main.ts
Outdated
| @@ -119,10 +121,12 @@ query openPullRequests($owner: String!, $repo: String!, $after: String) { | |||
| // we don't add it again because we assume that the removeOnDirtyLabel | |||
| // is used to mark a PR as "merge!". | |||
| // So we basically require a manual review pass after rebase. | |||
| core.setOutput(isPrDirtyOutputKey, false); | |||
There was a problem hiding this comment.
How does this work if multiple PRs are checked?
There was a problem hiding this comment.
At this time the last pr will count. But I wasn't sure why code was looping here. As far as I understand there can be only one PR per workflow instance because of how triggers work, correct?
We could return a map of pr number and is dirty instead, it just that the conditions people will have to write become more complex.
There was a problem hiding this comment.
A single push to master can invalidate multiple PRs. We should define what output we expect from these states and why.
There was a problem hiding this comment.
But that will trigger multiple instances of the workflow and each instance of the workflow will be only for one PR no?
There was a problem hiding this comment.
Why would it? It gets triggered for a push on main. Do github actions run on every open PR for a single push on main? This wasn't the case when I created the action. Say I have 30 open PRs and push on master. Only a single workflow is triggered on the master branch.
There was a problem hiding this comment.
after checking the readme again I noticed that the sample workflow triggers on push as well. I'm not sure why it's needed by it'd explain why this action supports multiple PRs at once.
I consequently updated this PR to return an array instead of a single value. Let me know what you think!
There was a problem hiding this comment.
I'm not sure why it's needed by it'd explain why this action supports multiple PRs at once.
How would you go about it? Let's say I have an open PR that is up-to-date with main. Now I push to main and the PR is now conflicting. Which workflow should mark this PR as outdated?
There was a problem hiding this comment.
here is my understanding of how GitHub actions triggers work and so far my experience is consistent with it, but I might be wrong. Have a look at https://github.com/microsoftgraph/microsoft-graph-docs/actions?query=workflow%3APullRequestConflicting you'll notice I'm only using pull_request synchronize event, not the push event.
I believe the synchronize event is triggered:
- anytime something gets pushed to the source branch
- anytime something gets pushed to the target branch
- anytime the target branch get's changed (targeting another branch)
Which means in my case that the workflow is triggered once per pull request on any of those events described above. When it's triggered in that context, only a single PR is in the action context.
So in that context, we could potentially update the code to query for that single PR, and to return (output) a single value.
That'd be a breaking change for people using the push trigger. And I might be misunderstanding something at this point.
There was a problem hiding this comment.
anytime something gets pushed to the target branch
That is not documented. What I found directly contradicts this statement:
This will NOT trigger if the base ref is updated.
-- https://github.hscsec.cnmunity/t/what-is-a-pull-request-synchronize-event/14784/4?u=eps1lon
You can verify this by setting up a new repo, push the initial, create a pr from the initial and then push to main. According to you the pull_request sync event should be fired on the push to main.
There was a problem hiding this comment.
ok thanks for clarifying! I probably got confused because of the sheer volume of activity we get in the repo I linked.
So I'd need to add the push trigger in my workflow. I'm assuming it's ok to filter on main branch? (all PRs target that branch)
Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>
eps1lon
changed the title
There was a problem hiding this comment.
Build passes (https://github.com/eps1lon/actions-label-merge-conflict/runs/890110526) and seems to work as expected: https://github.com/eps1lon/actions-label-merge-conflict/runs/890110526?check_suite_focus=true#step:9:69
|
@baywet Thanks! |
No description provided.