[Feature] check status of commits #30

MeghalBisht · 2021-11-17T17:47:14Z

Closes #20

When --commit flag is specified, status of commits are checked for success, failure or pending.

Need some discussion around the point mentioned in the issue

Why should failure of a commit status make a PR not mergeable?

Currently adding a --commit flag would show the status of commits but the PR mergeable logic stays the same.
(--commit can be considered as a flag to see the status of commits as well maybe? But not change the logic of mergeable PRs)

ljharb · 2021-11-17T21:23:14Z

The purpose of #20 is so that a commit that's pushed up, but is not part of a PR at all, can be checked.

If a commit is part of a PR, then I suppose the status checks on the commit could be used instead of those on the PR, because there are some cases where that's different.

I think it maybe makes sense for --commit to do two things:

also check commit statuses
make having a PR at all not be a requirement

bin/can-merge

utils/evaluateCommitStatus.js

MeghalBisht · 2021-11-18T17:05:05Z

The purpose of #20 is so that a commit that's pushed up, but is not part of a PR at all, can be checked.

~~So, the --commit type would actually be a string and not a boolean as mentioned in the issue, correct?~~ Discussed in the meeting.

bin/can-merge

utils/buildQuery.js

utils/evaluateCommitStatus.js

ljharb · 2021-12-03T06:40:54Z

@MeghalBisht heads up that i've rebased this; you may want to check out the branch fresh :-)

MeghalBisht · 2021-12-09T17:05:31Z

@ljharb This looks ready to land, can you give it a final look please?

ljharb

Getting close! I've rebased this for you; you'll want to delete your local branch and check it out again, or git reset --hard origin/commit-status after fetching.

bin/can-merge

ljharb · 2021-12-09T21:09:22Z

hmm, i just tried this on a commit, and it found no checks. This was the content of response:

{
  repository: { branchProtectionRules: { nodes: [] }, commit: null },
  rateLimit: { cost: 1, remaining: 4994 }
}

MeghalBisht · 2021-12-12T12:06:06Z

hmm, i just tried this on a commit, and it found no checks. This was the content of response:
{
  repository: { branchProtectionRules: { nodes: [] }, commit: null },
  rateLimit: { cost: 1, remaining: 4994 }
}

Hmm can you share the command you ran? I tested out a few commit SHAs, it seems to be working fine

bin/can-merge

ljharb

This looks great! I'll give it a try today and report any feedback.

ljharb · 2021-12-22T17:06:50Z

bin/can-merge

@@ -98,13 +105,26 @@ runQuery(args.pr, String(args.repo), args.sha, token).then((response) => {
 		console.log(JSON.stringify(response, null, 2));
 	}
 	const prs = parsePullRequest(response.repository);
-	if (prs.length === 0) {
+	if (args.checkCommitStatus && args.sha && !args.pr) {


it might be a good idea to check the commit status even if a PR is provided, but let's look into that after this PR lands.

ljharb · 2021-12-22T17:07:23Z

bin/can-merge

+				let status = null;
+				if (pr.commits.nodes.length > 1) {
+					throw chalk.red('Failed to evaluate the status of the commit');
+				}
+				status = evaluateCommitStatus(pr.commits.nodes[0].commit);


Suggested change

let status = null;

if (pr.commits.nodes.length > 1) {

throw chalk.red('Failed to evaluate the status of the commit');

}

status = evaluateCommitStatus(pr.commits.nodes[0].commit);

if (pr.commits.nodes.length > 1) {

throw chalk.red('Failed to evaluate the status of the commit');

}

const status = evaluateCommitStatus(pr.commits.nodes[0].commit);

ljharb · 2021-12-23T06:09:27Z

hmm - I ran this on sha d004c6b064 in https://github.com/nvm-sh/nvm, which i'd just force pushed to a PR, and I got:

⚠ This remote repository does not contain any pull requests matching sha: d004c6b064

response.repository is:

{"branchProtectionRules":{"nodes":[{"requiresStatusChecks":true,"requiredApprovingReviewCount":1,"requiredStatusCheckContexts":["Automatic Rebase","Travis CI - Pull Request","Require “Allow Edits”","dockerfile_lint","doctoc","eclint","nvm install-latest-npm","release","shellcheck","test_naming","nvm_windows"]}]},"commit":{"statusCheckRollup":{"state":"PENDING","contexts":{"totalCount":79,"pageInfo":{"endCursor":"Nzk","hasNextPage":false},"nodes":[{"__typename":"CheckRun","status":"IN_PROGRESS","name":"eclint","conclusion":null},{"__typename":"CheckRun","status":"COMPLETED","name":"Automatic Rebase","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"Require “Allow Edits”","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"MSYS fail prefix nvm install","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"matrix","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"release","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"shellcheck_matrix (bash, nvm.sh)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"shellcheck_matrix (sh, nvm.sh)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"shellcheck_matrix (dash, nvm.sh)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"shellcheck_matrix (ksh, nvm.sh)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"shellcheck_matrix (bash, install.sh)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"shellcheck_matrix (bash, bash_completion)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"shellcheck_matrix (bash, nvm-exec)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (17)","conclusion":null},{"__typename":"CheckRun","status":"QUEUED","name":"shellcheck","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"dockerfile_lint","conclusion":null},{"__typename":"CheckRun","status":"COMPLETED","name":"MSYS nvm install (--lts)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (16)","conclusion":null},{"__typename":"CheckRun","status":"COMPLETED","name":"MSYS nvm install (--default 12)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (15)","conclusion":null},{"__typename":"CheckRun","status":"COMPLETED","name":"MSYS nvm install (--no-progress 10)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (14)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"nvm install-latest-npm (13)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"nvm install-latest-npm (12)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"nvm install-latest-npm (11)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"nvm install-latest-npm (10)","conclusion":null},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (9)","conclusion":null},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (8)","conclusion":null},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (7)","conclusion":null},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (6)","conclusion":null},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (5)","conclusion":null},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (4)","conclusion":null},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (3)","conclusion":null},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (2)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"nvm install-latest-npm (1)","conclusion":null},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (9.2)","conclusion":null},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (9.1)","conclusion":null},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (9.0)","conclusion":null},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (6.1)","conclusion":null},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (5.9)","conclusion":null},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (4.6)","conclusion":null},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (4.5)","conclusion":null},{"__typename":"CheckRun","status":"QUEUED","name":"nvm install-latest-npm (4.4)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"nvm install-latest-npm (0.12)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"nvm install-latest-npm (0.10)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"doctoc","conclusion":null},{"__typename":"CheckRun","status":"COMPLETED","name":"Cygwin nvm install","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Debian, --lts)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"test_naming","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Debian, --lts, script)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Debian, 14)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Debian, 14, script)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Debian, 12)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Debian, 12, script)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Debian, 11)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Debian, 11, script)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Debian, 10)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Debian, 10, script)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Alpine, --lts)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Alpine, --lts, script)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Alpine, 14)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Alpine, 14, script)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Alpine, 12)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Alpine, 12, script)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Alpine, 11)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Alpine, 11, script)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Alpine, 10)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"COMPLETED","name":"WSL nvm install (Alpine, 10, script)","conclusion":"SUCCESS"},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"WSL nvm install (Ubuntu-18.04, --lts)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"WSL nvm install (Ubuntu-18.04, --lts, script)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"WSL nvm install (Ubuntu-18.04, 14)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"WSL nvm install (Ubuntu-18.04, 14, script)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"WSL nvm install (Ubuntu-18.04, 12)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"WSL nvm install (Ubuntu-18.04, 12, script)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"WSL nvm install (Ubuntu-18.04, 11)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"WSL nvm install (Ubuntu-18.04, 11, script)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"WSL nvm install (Ubuntu-18.04, 10)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"WSL nvm install (Ubuntu-18.04, 10, script)","conclusion":null},{"__typename":"CheckRun","status":"IN_PROGRESS","name":"Travis CI - Pull Request","conclusion":null}]}},"associatedPullRequests":{"edges":[]}}}

How is it unable to infer the PR?

ljharb · 2021-12-23T06:11:30Z

When I ran it with --commit, it correctly printed ⌛ Some commit checks are pending, but it exited zero (successfully) instead of nonzero.

ljharb

I've rebased this, and will try it out soon, and either provide feedback or land it :-)

ljharb · 2021-12-30T21:35:10Z

(I think #37 needs to be addressed before I can properly test this)

ljharb · 2022-01-06T00:05:49Z

Unfortunately landing #40 seems to have made this PR a bit tricky to rebase. @Green-Ranger11 or @MeghalBisht, want to take a crack at it?

Green-Ranger11 · 2022-01-10T21:29:41Z

Changes will have to be made to buildQuery since we are now using the search api and I'll work on that for now

ljharb · 2022-01-28T23:05:59Z

@Green-Ranger11 any update on rebasing this? I often need the --commit feature :-)

Green-Ranger11 · 2022-01-29T12:21:34Z

I've given it a shot and had to make some changes to the query. I've also added a flag -c for checking only the commit checks rather than the pull request checks

Green-Ranger11 · 2022-01-29T12:25:39Z

So if you add the --commit flag it will only check the status of the commit. If it's omitted can-merge will try to infer a pr to get checks.

Green-Ranger11 · 2022-01-29T12:38:14Z

One other thing is the watch flag for commits which is something I will work on,

ljharb · 2022-01-29T16:00:43Z

The intention is that with the commit flag, both are checked.

Green-Ranger11 · 2022-01-29T18:34:30Z

I'll add that in to check both pr checks and commit checks

ljharb · 2022-01-30T06:06:32Z

utils/evaluatePending.js


-	return pending.length;
+	return pullRequestPending.length === 0 ? false : true || commitRequestPending;


this reads a bit confusingly; can it be restated to avoid the false/true and/or the ternary+or?

Yeah I'll refactor it into a way that is readable in the next commit.

ljharb · 2022-01-30T06:07:08Z

utils/watch.js

@@ -7,7 +7,7 @@ module.exports = async function watch(retryDelay, getResponse) {

 	const pendingChecks = evaluatePending(response);

-	if (pendingChecks === 0) {
+	if (!pendingChecks) {


i prefer the comparison to zero; it’s more obvious what value pendingChecks contains

Because we now also have commit checks I can't equate it to zero. What if I rename it to isPendingChecks? Or I could just return both values from evaluatePending()
Thoughts?

Ah, if it’s a boolean then an “is” prefix works fine.

ljharb requested changes Nov 17, 2021

View reviewed changes

bin/can-merge Outdated Show resolved Hide resolved

utils/evaluateCommitStatus.js Outdated Show resolved Hide resolved

ljharb reviewed Nov 22, 2021

View reviewed changes

bin/can-merge Outdated Show resolved Hide resolved

bin/can-merge Outdated Show resolved Hide resolved

utils/buildQuery.js Outdated Show resolved Hide resolved

utils/evaluateCommitStatus.js Show resolved Hide resolved

MeghalBisht force-pushed the commit-status branch 3 times, most recently from 0d0f4e7 to 7366241 Compare November 26, 2021 17:35

ljharb force-pushed the commit-status branch from a075dce to 8217d7d Compare December 3, 2021 06:40

ljharb force-pushed the commit-status branch from c103347 to 78c6fa0 Compare December 9, 2021 17:55

ljharb requested changes Dec 9, 2021

View reviewed changes

bin/can-merge Outdated Show resolved Hide resolved

MeghalBisht commented Dec 12, 2021

View reviewed changes

bin/can-merge Outdated Show resolved Hide resolved

ljharb mentioned this pull request Dec 16, 2021

[New] add --watch to poll for changes every N seconds #9

Merged

ljharb reviewed Dec 22, 2021

View reviewed changes

ljharb force-pushed the commit-status branch from e49b6fe to 325b948 Compare December 25, 2021 20:42

ljharb reviewed Dec 25, 2021

View reviewed changes

ljharb force-pushed the commit-status branch from 325b948 to c23c65e Compare December 26, 2021 05:48

Green-Ranger11 force-pushed the commit-status branch from c23c65e to 5ac4982 Compare January 29, 2022 12:20

Green-Ranger11 force-pushed the commit-status branch from 5ac4982 to 848a8db Compare January 29, 2022 12:27

Green-Ranger11 force-pushed the commit-status branch from 848a8db to 988a6c1 Compare January 29, 2022 12:33

Green-Ranger11 force-pushed the commit-status branch 4 times, most recently from 85a9c34 to 615b7d0 Compare January 30, 2022 06:03

ljharb reviewed Jan 30, 2022

View reviewed changes

[New] add --commit to check status of commits instead of just PRs

9e65a6a

Green-Ranger11 force-pushed the commit-status branch from 615b7d0 to 9e65a6a Compare January 30, 2022 21:26

Green-Ranger11 requested a review from ljharb January 31, 2022 00:23

ljharb merged commit 9e65a6a into main Feb 2, 2022

ljharb deleted the commit-status branch February 2, 2022 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] check status of commits #30

[Feature] check status of commits #30

MeghalBisht commented Nov 17, 2021 •

edited

Loading

ljharb commented Nov 17, 2021

MeghalBisht commented Nov 18, 2021 •

edited

Loading

ljharb commented Dec 3, 2021

MeghalBisht commented Dec 9, 2021

ljharb left a comment

ljharb commented Dec 9, 2021

MeghalBisht commented Dec 12, 2021

ljharb left a comment

ljharb Dec 22, 2021

ljharb Dec 22, 2021

ljharb commented Dec 23, 2021

ljharb commented Dec 23, 2021

ljharb left a comment

ljharb commented Dec 30, 2021

ljharb commented Jan 6, 2022

Green-Ranger11 commented Jan 10, 2022

ljharb commented Jan 28, 2022

Green-Ranger11 commented Jan 29, 2022 •

edited

Loading

Green-Ranger11 commented Jan 29, 2022

Green-Ranger11 commented Jan 29, 2022

ljharb commented Jan 29, 2022

Green-Ranger11 commented Jan 29, 2022

ljharb Jan 30, 2022

Green-Ranger11 Jan 30, 2022

ljharb Jan 30, 2022

Green-Ranger11 Jan 30, 2022

ljharb Jan 30, 2022


		return pending.length;
		return pullRequestPending.length === 0 ? false : true \|\| commitRequestPending;

[Feature] check status of commits #30

[Feature] check status of commits #30

Conversation

MeghalBisht commented Nov 17, 2021 • edited Loading

ljharb commented Nov 17, 2021

MeghalBisht commented Nov 18, 2021 • edited Loading

ljharb commented Dec 3, 2021

MeghalBisht commented Dec 9, 2021

ljharb left a comment

Choose a reason for hiding this comment

ljharb commented Dec 9, 2021

MeghalBisht commented Dec 12, 2021

ljharb left a comment

Choose a reason for hiding this comment

ljharb Dec 22, 2021

Choose a reason for hiding this comment

ljharb Dec 22, 2021

Choose a reason for hiding this comment

ljharb commented Dec 23, 2021

ljharb commented Dec 23, 2021

ljharb left a comment

Choose a reason for hiding this comment

ljharb commented Dec 30, 2021

ljharb commented Jan 6, 2022

Green-Ranger11 commented Jan 10, 2022

ljharb commented Jan 28, 2022

Green-Ranger11 commented Jan 29, 2022 • edited Loading

Green-Ranger11 commented Jan 29, 2022

Green-Ranger11 commented Jan 29, 2022

ljharb commented Jan 29, 2022

Green-Ranger11 commented Jan 29, 2022

ljharb Jan 30, 2022

Choose a reason for hiding this comment

Green-Ranger11 Jan 30, 2022

Choose a reason for hiding this comment

ljharb Jan 30, 2022

Choose a reason for hiding this comment

Green-Ranger11 Jan 30, 2022

Choose a reason for hiding this comment

ljharb Jan 30, 2022

Choose a reason for hiding this comment

MeghalBisht commented Nov 17, 2021 •

edited

Loading

MeghalBisht commented Nov 18, 2021 •

edited

Loading

Green-Ranger11 commented Jan 29, 2022 •

edited

Loading