-
-
Notifications
You must be signed in to change notification settings - Fork 777
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
DO NOT REVIEW: GitHub Actions: Implement ESlint #5024
base: gh-pages
Are you sure you want to change the base?
DO NOT REVIEW: GitHub Actions: Implement ESlint #5024
Conversation
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes.
|
@averdin2 @macho-catt, is this okay? |
We could also look into using super-linter/super-linter/slim@v5 since we aren't using all the rules, and the file size would be slightly smaller. |
Hi @ronaldpaek you showed a screenshot in which ESLint failed due to a variable that had been declared but not used. Would that cause merging to be blocked? |
I can verify and check, I think it would probably want you to fix it, but you can always put the es-lint disable comment to basically tell es-lint to ignore what's between these lines or the next line so it would let the developers know you explicitly told eslint to ignore this. |
@roslynwythe so that it will fail and you will get an error, and you can always bypass it with eslint disable rule if you want, but let's say you open an existing file on the project that has already been pushed. It will simply show you a warning/error in the problems tab in the terminal, but once you close the file, you won't see the error, so it just lets you know potential problems. |
5c5657a
to
62900f5
Compare
@ronaldpaek The screenshots above show alerts regarding an unused variable which was detected by both CodeQL scanner and the ESLint js scanner. CodeQL displays a code annotation with the error along with an option for dismissing the alert but doesn't block merging, because this error is not level HIGH or CRITICAL. We have configured CodeQL so that alerts of level Medium and lower do not block merging. Please advise: |
@ronaldpaek Please advise, if MegaLinter could be used in place of super-linter? |
@roslynwythe yes you can set up eslint to just provide warnings but not throw errors. "off" or 0 - turn the rule off
Online states that there is key differences in eslint vs CodeQL. ESLint: Primarily a linting tool for identifying and reporting on patterns in JavaScript. While it does have some security-focused plugins and rules (especially when paired with plugins like eslint-plugin-security), its main focus is on code quality and style. CodeQL: A semantic code analysis engine. It treats code as data and allows you to run queries against it. Its primary focus is on identifying security vulnerabilities in the codebase. Yes, MegaLinter can be used as an alternative to super-linter. super-linter: An out-of-the-box linter tool by GitHub, which is basically a combination of various linting tools into a Docker container. Its main advantage is the ease of setup for GitHub Actions. MegaLinter: It can be seen as an enhanced super-linter. It supports more languages and linters than super-linter and provides richer feedback, including fixing some issues automatically. It also provides a variety of output formats and integrations. |
Fixes #1442
What changes did you make?
Why did you make the changes (we will use this info to test)?