-
-
Notifications
You must be signed in to change notification settings - Fork 298
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
Ensure GitHub status checks have passed #289
Conversation
Thanks for starting this 🙌 |
I agree with your list of missing things.
Use |
I often do some local commits. Usually some nitpicks before doing a release. And I then run |
👍 |
870da36
to
4f938f5
Compare
4f938f5
to
4650f5a
Compare
4a29c15
to
d5c0f80
Compare
ef74c8d
to
bc8d011
Compare
It’s getting somewhere 🙌 The checklist is now done but I still need to thoroughly test every case. Nevertheless, I think this is ready for another code review. I hope the commits along with my intentions are easy to follow. If there’s anything unclear, let me know. Edit: I’ve now manually tested quite a few cases, such as commits with pending checks, commits with multiple failed checks, rate limit exceptions, and timeouts. |
- If there’s only one check, GitHub shows “Failure: ” and the check’s description - If there are multiple checks and only some failed, GitHub shows “Some checks were not successful” and a list of all checks - If there are multiple checks and all failed, GitHub shows “All checks have failed” and a list of all checks
bc8d011
to
f6e3290
Compare
index.js
Outdated
title: 'Ensure GitHub checks have passed', | ||
skip: () => { | ||
if (!pkg.repository) { | ||
return 'No repository specified'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No repo specified in package.json
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about “No repository specified in package.json”? The package.json
field is also called repository
so I’m not sure whether we should abbreviate here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“No repository specified in package.json”? The package.json field is also called repository so I’m not sure whether we should abbreviate here.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: c12e272.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should np fallback to getting the repo URL from git itself? I feel like requiring pkg.repository
is just needless busy work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve had the same thought at one point. I don’t have particularly strong opinions in either direction.
On one hand, requiring pkg.repository
encourages best practices (for example, npmjs.com will display a link to the repository if the field is specified) and gives us higher assurance that it actually specifies the package’s repository (whereas the Git remote might be a fork). On the other hand, this task already kind of expects the Git remote to be the upstream repository (since we git push
before querying the GitHub API).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an interesting thought. If I'm on a fork and running np
, I'm probably intentionally trying to publish some hacky thing I did, perhaps to a scoped package under my own personal namespace. I'm more likely to have a PR open on the pkg.repository
(which I probably haven't updated and where CI checks are probably running) than to have CI setup on my fork. I don't think I have ever set up CI on a fork, though major projects obviously would. 🤔
That being said, I still think if pkg.repository
isn't defined at all, it might be worth having some kind of fallback. Just "thinking out loud" here.
source/github-checks.js
Outdated
// It might take a bit for the status checks to be created. | ||
// Even if, it probably takes at least a few seconds for them to pass. | ||
task.title = `${title} ${chalk.yellow('(waiting for checks to start…)')}`; | ||
await delay(THIRTY_SECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't 10-15s suffice in this case?
Also, is this really needed? Won't pWaitFor
do this for us anyways?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't 10-15s suffice in this case?
Probably. I just sticked to the existing delay to keep it simple. If there are actually any status checks, I’d expect them to take more than 30 seconds to complete for most projects anyway.
Also, is this really needed? Won't
pWaitFor
do this for us anyways?
If we immediately call GitHub’s status API, total_count
might still be zero and we’d skip the task with “No status checks found”. On the flip side, we can’t retry in that case because commits without any status checks would be caught in a loop then (since the state
will always be pending
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't 10-15s suffice in this case?
I agree. 30 seconds is too long.
It might take a bit for the status checks to be created.
This sounds like a GitHub bug. Why wouldn't they be created right away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. 30 seconds is too long.
Changed to 10 seconds in c704b6c.
This sounds like a GitHub bug. Why wouldn't they be created right away?
As far as I understand, GitHub can’t know whether a commit will result in status checks being created. For example, GitHub only notifies Travis CI of Git pushes and it’s then Travis CI’s call whether to create any status checks (which it might not need to, depending on your settings).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. What about GitHub Actions? I assume GitHub would know then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub could but I still think there would be a slight delay. According to GitHub Help, GitHub Actions uses the Checks API to output statuses. Therefore, I think the Statuses API would still return zero status checks in the time frame between the commit having been pushed and the workflow starting to run.
@sonicdoe Is this done? If so, I think it would be beneficial to merge it and get it out there so people can try it out and report any issues. |
@sindresorhus Sounds good, I’m fine with giving it a go 🙂 |
I just tried this on https://github.com/sindresorhus/is-interactive after pushing a commit, but it skipped the task:
However, there clearly is a status check: |
It looks like it took over ten seconds for the status check to be created. I don’t think GitHub exposes the push time so it’s difficult to say for sure. I can only see that the commit was committed at 07:37:04 and the Travis CI build started at 07:39:06. If that’s the case, I’m not sure how to best resolve this. We don’t know when a check will be created and we can’t know in advance whether any checks will be created at all 😕 If it’s any improvement, we could check whether the latest commit (before pushing new ones) has checks and use that as an indicator. |
If I understand the code correctly, it currently delays 10 seconds, then tries to check the status, and if Here's how I would approach this:
|
Interestingly, Get status checks protection is only available to authenticated users (see this topic). Fortunately, Get a branch provides enough information in the In any case, I’m not sure how widespread protected branches are. For example, neither lodash/lodash, nor facebook/react, nor chalk/chalk list required status checks. Most importantly, sindresorhus/is-interactive doesn’t have any. |
That's why retries are so important. It doesn't currently retry when I do think that more people would enable branch protection if |
We’ve actually had a delay of 30 seconds before (see this conversation). I guess it will always be a balance between not missing any status checks and annoying people who don’t use them at all.
How would retrying up to one minute when there are no branch protection rules motivate people to enable them? People who don’t use status checks couldn’t enable protection and therefore would always have to wait a whole minute. On the flip side, people who do use status checks wouldn’t really gain much from enabling protection because they’d have to wait for their status checks to appear in both cases. Granted, the second group would have the minor benefit of |
It's about reliability. Take Sindre, for example. As you mentioned, he doesn't currently use branch protection. With the algorithm I proposed, it'll still work for his repos as-is, but if there's an unusually high delay, as happens occasionally (on bad days I've seen it take a couple of minutes to create the status), then Additionally, I suspect that a lot of people may just not be aware of the branch protection feature and its ability to guarantee a status check has passed. It's not exactly front and center in the GitHub UI. There are good reasons to enable it that have nothing to do with
No, not always. I'm not proposing a one minute delay. It's a timeout. It's only a whole minute in the worst case. |
Thanks for elaborating. I’d even go so far as to fail if
I don’t think I follow. If you don’t use status checks at all, in which case would your proposed algorithm skip sooner than one minute? |
Is anyone interested in pushing this forward? This is not something I personally need, so I don't really have a lot of energy to make decisions and push this forward. |
I'm unfortunately going to close this as it doesn't seem to go anywhere: #53 (comment) |
No worries, thanks for everything so far! The fact that status checks are created asynchronously, which I think is the biggest unresolved issue, is certainly unfortunate. |
Ensures GitHub status checks have passed using the Statuses API. Resolves #53.
This is a really quick implementation to gather feedback early. Some things that are missing or need to be discussed:
--no-checks
)