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.

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

Merged
merged 5 commits into from May 21, 2024

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented May 8, 2024

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

aduh95 added 2 commits May 8, 2024 14:41
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
Copy link
Member

@MoLow MoLow left a 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?

@aduh95
Copy link
Contributor Author

aduh95 commented May 8, 2024

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.

async checkCommitsAfterReviewOrLabel() {
if (this.checkCommitsAfterReview()) return true;

await Promise.all([this.data.getLabels(), this.data.getCollaborators()]);

This comment was marked as resolved.

Copy link
Contributor Author

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.

@fahrradflucht
Copy link
Contributor

I created a PR against this branch on your fork (aduh95#1) 🙂

@aduh95
Copy link
Contributor Author

aduh95 commented May 18, 2024

/cc @nodejs/node-core-utils

@aduh95 aduh95 merged commit 6cc2b1a into nodejs:main May 21, 2024
10 of 11 checks passed
@aduh95 aduh95 deleted the request-ci-certify-safe branch May 21, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allowing using request-ci label for collaborators in unapproved PRs
4 participants