-
Notifications
You must be signed in to change notification settings - Fork 114
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
feat(ncu-ci): check for request-ci
label
#806
Conversation
When running `ncu-ci` without the `--certify-safe` CLI flag, if something was pushed to a PR since the last approving review, check if the last time the `request-ci` label was added, it was done by a Collaborator and after the last push event on the PR.
fix for when there are no request-ci labels
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.
LGTM. can you add tests?
Tests would be great, I'd prefer if someone else could contribute that, I'm not sure when I'd have time to work on that |
lib/pr_data.js
Outdated
@@ -90,6 +91,14 @@ export default class PRData { | |||
]); | |||
} | |||
|
|||
async getLabels() { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
lib/pr_checker.js
Outdated
async checkCommitsAfterReviewOrLabel() { | ||
if (this.checkCommitsAfterReview()) return true; | ||
|
||
await Promise.all([this.data.getLabels(), this.data.getCollaborators()]); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
How would that change the testability? Making extra requests will make it more likely that the bot will reach its quota of requests and be rate limited, making the requests lazilly seems like a low-hanging fruit.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
I created a PR against this branch on your fork (aduh95#1) 🙂 |
/cc @nodejs/node-core-utils |
When commits were pushed to a PR since the last approving review, check if the last time a
request-ci
label was added, it was done by a Collaborator and after the last push event on the PR.I don't really like the fact
"request-ci"
is hard-coded, and also it might make sense to enable this check only in the GHA workflow (as the code doesn't search for'unlabeled'
events, so if someone adds the label and removes it, it would still still count), so if you have suggestions regarding that, I'd be interested.Fixes: #801