-
Notifications
You must be signed in to change notification settings - Fork 166
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
Disable Jenkins CI / git node request CI when no approvals #3696
Comments
That doesn't seem desirable, it's useful to be able to start CI runs – that's even a reason often mentioned when suggesting onboarding new collaborators. |
Well ci.nodejs.org and git node uses your own GitHub credentials, which are validated and authorized within Jenkins. |
I am -1 because that means no CI can be run to verify that it even works across the platform without an approval. And "approving to land" vs "approving to run in the CI" are completely different things. It also means to get a change that needs to handle platform specifics working, you have to get a PR up and going which may attract unwanted comments in a WIP that's not ready for review (in the issue tracker, even if you put |
Also, how much security does this offer? Is it assuming that collaborators can get their GitHub account compromised to start Jenkins? In that case they can also work around it by pushing to some old PR (opened by others) that no one pays attention to, approving the change themselves, and start a CI on it. Or create a fake account to open a PR, and approve it using the collaborator account to run the CI on it. |
I personally think we should revert the changes for the reasons highlighted in I don’t want to wait for another collaborator to LGTM my changes to be able to run them on Windows, AIX, ARM or other odd environments. This will slow us down significantly. |
Recently, I received the following message: nodejs/node#52761 (comment) and that's great! However, it doesn't cover all the cases such as requesting it through https://ci.nodejs.org/ or
git node
. As a security measure, how hard would be to include both in these rules?cc: @nodejs/security-wg
The text was updated successfully, but these errors were encountered: