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

Allowing using request-ci label for collaborators in unapproved PRs #801

Closed
rluvaton opened this issue Apr 27, 2024 · 6 comments · Fixed by nodejs/node#52940 or #806
Closed

Allowing using request-ci label for collaborators in unapproved PRs #801

rluvaton opened this issue Apr 27, 2024 · 6 comments · Fixed by nodejs/node#52940 or #806

Comments

@rluvaton
Copy link
Member

rluvaton commented Apr 27, 2024

Hey everyone, we now can't run CI for unapproved PRs using the request-ci label

⚠  No approving reviews found 
✘  Refusing to run CI on potentially unsafe PR

I believe it was done to avoid security risks for running untrusted code in the CI but collaborators can pass that by starting the CI using ci.nodejs.org

And this is really inconvenient for collaborators

So can we change it so collaborators can run the CI using the request-ci label

@aduh95 aduh95 transferred this issue from nodejs/TSC Apr 27, 2024
@mcollina
Copy link
Member

+1, we need a different mechanism

@aduh95
Copy link
Contributor

aduh95 commented Apr 27, 2024

My reasoning when implementing the security check was there was no reason for a collaborator to start a CI on an unapproved CI (likely reviews will have nits that you'd want to merge before starting CI) – and also it was simpler to not treat collaborators differently.

@atlowChemi
Copy link
Member

My reasoning when implementing the security check was there was no reason for a collaborator to start a CI on an unapproved CI (likely reviews will have nits that you'd want to merge before starting CI) – and also it was simpler to not treat collaborators differently.

@aduh95 perhaps we could change it to start CI execution on PR with an approval, even if not on latest commits, so this way if you fixed nits etc, you don't need a re-approval?

@mcollina
Copy link
Member

My reasoning when implementing the security check was there was no reason for a collaborator to start a CI on an unapproved CI (likely reviews will have nits that you'd want to merge before starting CI) – and also it was simpler to not treat collaborators differently.

Well, no. Myself (and many others) open PRs to verify if the change fails on all the list of environments we support. Waiting for an approval to start CI will slow development significantly, and it basically tell maintainers to start the CI manually completely defeating the purpose of the label.

@mcollina
Copy link
Member

I recommend the change to be reverted while we find a different solution.

@mcollina
Copy link
Member

I've opened nodejs/node#52940.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants