-
Notifications
You must be signed in to change notification settings - Fork 41
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
Issues/252 megalinter #253
Conversation
🦙 MegaLinter status: ❌ ERROR
See detailed report in MegaLinter reports You could have same capabilities but better runtime performances if you request a new MegaLinter flavor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a net positive change to me: smaller Docker images, parallel runs, easy links to errors via a comment, and other improvements over super-linter
. 👍
My only question is on the total runtime, it seems like super-linter
took 13-15 mins to run, whereas megalinter is taking 15-18 mins to run, specifically because of the terraform_kics
tests. What is taking so long in those tests, is it spinning up infra (or mock infra)?
I wonder if it would be worthwhile to disable terraform_kics
tests, then the linting would take ~70 seconds (based on the next-slowest-running test). Then you could configure a "nightly" (or similar infrequent) run of the tests that includes compliance with terraform_kics
.
# Trigger mega-linter at every push. Action will also be visible from Pull Requests to master | ||
push: # Comment this line to trigger action only on pull-requests (not recommended if you don't pay for GH Actions) | ||
pull_request: | ||
branches: [master, main] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this repo has a master
branch, correct?
@koverholt most repositories using MegaLinter disable some linters / rules / errors etc... for a lot of reasons I have no idea why KICS spends so much time on terraform files, but with all other linters checking terraform it would feel safe to disable it by adding the following in .mega-linter.yml config file DISABLE_LINTERS:
- TERRAFORM_KICS PYTHON_PYRIGHT and REPOSITORY_DEVSKIM are also known to raise lots of not so relevant issues... I usually disable them too About cspell, in the report artifacts zip you have an updated .cspell config that you can paste at the root of your repo, and by viewing the diff you'll see which are real words that needs to be accept and which are typos that should be corrected You can also decide to use MegaLinter gradually by defining some linters are not blocking using variable DISABLE_ERRORS_LINTERS More info here: https://megalinter.io/latest/configuration/#activation-and-deactivation |
About eslint issue, you could add the following in .mega-linter.yml config file PRE_COMMANDS:
- command: npm install eslint-plugin-json |
@nvuillam Wow what a coincidence, I was just doing exactly this! TY for reaching out!! |
@nvuillam |
@nicain the planets are aligned ;) About other linter issues, you can decide if you keep them enabled or not, blocking or not If you want to keep them enabled but disable some errors , each linter doc on MegaLinter has a direct link to the related linter page containing the doc explaining how to disable stuff ^^ Note: there is an awful bug in latest version, if you see a 404 when clicking to the doc, juste remove the version number of the url to keep "latest" |
There is a current issue oxsecurity/megalinter#1975 and a PR oxsecurity/megalinter#1985 , handled by @Kurt-von-Laven It has testing CI jobs failing... if you are a docker expert maybe you could help by providing an example of docker run command that would work on your config, and we could add an option to mega-linter-runner to activate it ! |
If it can help, the docker run command is built here -> https://github.com/oxsecurity/megalinter/blob/0234f93c064f6e5da9414dea7b764b640ede3613/mega-linter-runner/lib/runner.js#L123 You can also see it as it is run in the logs |
@koverholt I think the best way to move this forward is to:
If we use the same config files for both linters, we should have parity between the two functionalities, so we can ease a transition one piece at a time. WDYT? |
That sounds good to me! I'd be in favor of enabling the linters that are passing in this PR, then as a next pass (in another PR) enabling the linters that have < 5 errors and fixing them, then switching to megalinter as you mentioned. |
If I may, I suggest you to do the following:
|
FYI, url bug leading to 404 is fixed :) |
Going to close this PR for now and revisit in the future while I fix current CI tests in #383. |
Fix #252
Checking out megalinter; slight hiccup using npx mega-linter-runner because of my sudo-protected docker:
What I tried:
npx mega-linter-runner
What I got:
How I fixed it: https://stackoverflow.com/a/54504083
sudo setfacl --modify user:<user name or ID>:rw /var/run/docker.sock