failed non-required checks should not fail the overarching command

[ljharb]

I ran can-merge --pr=428 on ljharb/qs#428, and the two node 0.6 jobs failed (which is expected). Then the command exited with code 1.

However, since all of the required jobs have passed, it should have exited with code 0.

[Green-Ranger11]

Also linking this comment in #43 as it seems to also be similar in that can-merge is not checking whether status and checks are required before factoring it into its final status.

[Green-Ranger11]

Anyone thinking of grabbing this? I want to pick this one up please

[Green-Ranger11]

Just noting this down but my first approach was to get all branch protection rules for target branch but I kept getting empty objects from the api but it turns out if you don't have the relevant rights to the repo you can't view the branch protection rules (sigh this took me while to figure out).
However, the status from GitHub should be correct (as in if non-required failed) thus I'm going go down that avenue and see if there's a mistake in our logic especially in evaluatePullRequest

[ljharb]

Sounds great.

It might help to create a command-line tool in this repo - just for devs, not part of the published package - that I can use to dump API output to disk. That would allow:

• someone filing an issue to easily generate and include the API response needed to trigger it
• one person with access, to generate and provide the API response to a contributor without that access

[Green-Ranger11]

Yeah I kept wondering if I was losing my mind and went down rabbit holes until I tried querying a repo I own (which maddens me even more the fact I didn't do that earlier).
That being said, having a tool like that would really help.

[Green-Ranger11]

Some updates on my end:
GitHub API does not actually return the true status of the PR. The state property only refers to merge conflicts and does not take in account whether checks passed or not.
That being said, there is a way to check whether the checks are required or not as both CheckRun and StatusContext objects have a isRequired property however the pull request number has to be passed as a parameter. So an idea for getting that property could be: (Would also love any suggestions/alternatives too)

If pr number (--pr) specified
	Pass that into our query and get back the `isRequired` property.
Else
	Once the response comes back query again with the pr number returned in that object

I don't really like this as it requires us to waste 2 api points since non-owners cannot even view branch-protection-rules

[ljharb]

hm - the extra request isn't a huge deal, but you're saying that non-maintain/admin can't even see if a check is required or not?

[Green-Ranger11]

Yes that is correct. When you try to get branchProtectionRules object from GitHub you won't be able to return anything unless you're admin.
What you can do is return the already run checks which contains the isRequired property like I mentioned above.

[ljharb]

aha, ok. in that case, it seems like when the user is an admin of a repo, we'd have branch protection checks, and we're set! When not, and we have a PR number, we have isRequired; when we don't, we need to make an extra request.

However, if a check hasn't completed, do we know if it's required? It's useful to know that all the required checks are done, and the only pending ones are optional.

[Green-Ranger11]

Good question and my bad for not clarifying that in my previous reply!
Actually, the isRequired is returned on checks that actually start (because those are the checks that are returned from the api - checks not started we'll never know if they're required or not because api doesn't return them). So one one issue I see is that if some checks running are depend on others passing before running and those checks[the ones that run after] are required then we'll never know because we can only see if the check is required after it has started.

The best bet is to hope the one running can-merge is an admin OR all the required checks actually run so we can get the isRequired property.

This is why you'll see this strange behavior when running --watch flag where it says like 2 checks pending then you look again it has gone up to like 11 checks pending.

Lastly, sftl replies in this thread when I know you're following up.

[ljharb]

(i'm not sure what "sftl" means)

It's totally fine to start with 2 checks pending and jump to 11, eg - it's not ideal obviously, but at least it still provides the right answer.

So, to reiterate: if the user is an admin, we can check branch protections and get the complete list of required checks. If the user is not an admin, we can only know about checks that have "started" (existence, or required-ness). If we have a PR number, we can get this info on the initial request; if not, we have to make an additional request to get the info.

Is that accurate?

[Green-Ranger11]

sftl - sorry for the late.

Yes that is correct!

[ljharb]

Sounds like a fine plan, and as robust as is possible.

[Green-Ranger11]

Sounds great and working on that right now.

Will also open another issue afterwards as a reminder incase anything on GitHub's end changes .