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

RFC: make a "safe" job to do linting #1554

Closed
refack opened this issue Oct 31, 2018 · 6 comments
Closed

RFC: make a "safe" job to do linting #1554

refack opened this issue Oct 31, 2018 · 6 comments

Comments

@refack
Copy link
Contributor

refack commented Oct 31, 2018

The motivation is to have a job we can run for every PR, irrespective of the author.
In order to reduce the attack surface, I'm thinking of using a "fixed" node version to lint the checked out code (so the author doesn't have control of the linting code).

Ohh and actually compiling a report to post to the GitHub thread, instead of sending the user to a different (sometimes obscure) system.

Seeking more feedback.

@joaocgreis
Copy link
Member

Doesn't the Travis job already do this? Perhaps commenting from Travis is the easiest option on the table.

Having a "safe" job is not only about the binary, but not letting the PR opener control anything that runs. In this case, the Makefile. A quick way would be to copy the commands to the Jenkins job, but that'd be far from great to maintain and we should probably still run it in dedicated machines to be safe. What about using Docker? Something similar to the arm-fanned job but discarding the container after each run.

@refack
Copy link
Contributor Author

refack commented Nov 23, 2018

Doesn't the Travis job already do this?

So for example we're now hitting GitHub API limits on Travis as tracked in nodejs/node#24567
Also apparently PR jobs on Travis can't use project secrets.

@richardlau
Copy link
Member

The rate limits are being hit on Travis for the commit message linting, which currently uses the unauthenticated GitHub API. The commit message linting itself doesn't actually need to be run on a checked out copy of the PR code (unless the script itself is being changed).

The normal code/docs linting doesn't use the GitHub API and is thus not affected by rate limits.

@refack
Copy link
Contributor Author

refack commented Nov 23, 2018

The normal code/docs linting doesn't use the GitHub API and is thus not affected by rate limits.

But if I want to push feedback to the PR thread, I need to make an API call.

@github-actions
Copy link

github-actions bot commented Mar 6, 2020

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is remove or a comment is made.

@github-actions github-actions bot added the stale label Mar 6, 2020
@richardlau
Copy link
Member

Closing this out. We run linters on every PR using Travis and using GitHub Actions. With the actions we even have in place annotations for JavaScript lint failures and we could add the same in the future for markdown and cpplint failures.

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

No branches or pull requests

3 participants