oxsecurity / megalinter

πŸ¦™ MegaLinter analyzes 50 languages, 22 formats, 21 tooling formats, excessive copy-pastes, spelling mistakes and security issues in your repository sources with a GitHub Action, other CI tools or locally.
https://megalinter.io
GNU Affero General Public License v3.0
1.89k stars 228 forks source link

mega-linter-runner does not map LINTER_RULES_PATH to docker #157

Closed joe-sharp closed 3 years ago

joe-sharp commented 3 years ago

Describe the bug This is a somewhat two layer issue for the local version: mega-linter-runner. In my repository Mega-Linter reads linter config files from .github/linters, however when running mega-linter-runner locally the path is not used so I get a different set of rules. I think a parameter similar to the -p flag would probably be best and possibly required here, but I also tried manually passing the environment variable LINTER_RULES_PATH without success.

To Reproduce Steps to reproduce the behavior:

  1. Clone https://github.com/joe-sharp/treehouse-vue.git NOTE: make sure to recurse submodules or it will not have the config files: git clone https://github.com/joe-sharp/treehouse-vue.git --recurse-submodules
  2. Optionally fork it and test Mega-Linter GitHub Action works
  3. Run mega-linter-runner -e 'SHOW_ELAPSED_TIME=true' -e 'DEFAULT_BRANCH=main'
  4. Optionally add -e 'LINTER_RULES_PATH=.github/linters'

Expected behavior I expect .github/linters to be mapped to Docker by default, and additionally it would be nice to have an option of overriding the directory to say project root. (What happens if I passed a subdirectory in the project with the -p flag without this feature?)

Additional context Since you recently recommended I move my config to project root I think I should add context as to why I haven't. I have set up a repository: https://github.com/joe-sharp/linter-configs that I will incorporate as a submodule in other projects, allowing me to maintain consistent rules across all my projects. The repo you clone in the steps to reproduce is the first example of that. Also, if you don't mind waiting a few days tops, I plan on attempting to fix this and would be delighted to open a PR assuming I am successful, but I thought I would document the issue first. πŸ˜„

joe-sharp commented 3 years ago

GitHub Action:

2020-12-14 05:31:37,988 [INFO] ### Linting [HTML] files
- Using [htmlhint v0.14.2] https://htmlhint.com/
- Mega-Linter key: [HTML_HTMLHINT]
- Rules config: [/github/workspace/.github/linters/.htmlhintrc]

mega-linter-runner:

2020-12-14 06:04:00,053 [INFO] ### Linting [HTML] files
- Using [htmlhint v0.14.2] https://htmlhint.com/
- Mega-Linter key: [HTML_HTMLHINT]
- Rules config: [/action/lib/.automation/.htmlhintrc]
nvuillam commented 3 years ago

Thanks for the clear description, I'll have a look :) It probably shouldn't be too hard to solve ^^

nvuillam commented 3 years ago

And your "why I did not move configs at the root" makes me think of new feature... allowing LINTER_RULES_PATH to be remote ? At runtime it would just fetch the config files via HTTP ^^ Would you have a use of such feature ?

joe-sharp commented 3 years ago

It actually would be way better that way, because if I make changes to the config files and merge them into main in that repository, I then have to pull the changes into every other project.

nvuillam commented 3 years ago

Ok so first I fix the bug you declared, then I complete my PR about Mega-Linter flavors #156 to improve again performances, and then i'll see about remote config files :)

joe-sharp commented 3 years ago

You're the best! Thanks so much. Still loving the linter, will love it even more when I can can add a pre-push hook with mega-linter-runner that checks what I am about to push up. πŸ‘πŸΌ

nvuillam commented 3 years ago

You're the best! Thanks so much. Still loving the linter, will love it even more when I can can add a pre-push hook with mega-linter-runner that checks what I am about to push up. πŸ‘πŸΌ

About this one... except if you have a strong computer (it"s heavy to pull and run a docker image^^) , i still think that using linters in IDEs and use the config files at root level are best in terms of performances πŸ‘Ό But easier multi-repo shred config & maintenance with mega-linter-runner, indeed ^^

joe-sharp commented 3 years ago

Well I will definitely need to test it more once it is reading my config files, but currently it runs in that repository in just under 30 seconds. It might be a bit too painful for pre-push but I think it is worth a shot. This is with a newer but base model Intel Mac Mini:

  OS: macOS Mojave 10.14.6 18G6042
  Host: Macmini8,1
  Kernel: Darwin 18.7.0
  Terminal: iTerm2
  Packages: 55 (brew)
  Shell: /bin/zsh 5.3
                    Resolution: 2560x1440 @ 60Hz, 1080x1920 @ 60Hz
                    GPU: Intel UHD Graphics 630
                    GPU Driver: macOS Default Graphics Driver
                    CPU: Intel i5-8500B (6) @ 3.00GHz
                    Memory: 6195MiB / 8192MiB
nvuillam commented 3 years ago

My computer is becoming old... I rarely use locally mega-linter-runner ^^

About that... please could you run again your test, but with mega-linter-runner -e 'SHOW_ELAPSED_TIME=true' -e 'DEFAULT_BRANCH=main' -e 'LOG_LEVEL=DEBUG' and post the log ?

Note: you can also set all these variables in .mega-linter.yml so you don't have to define them twice on GHA Workflow and mega-linter-runner call ^^

joe-sharp commented 3 years ago

Sure! https://gist.github.com/joe-sharp/2b897d75401c0920fd6541a3fd41df8e Let me know if there is anything else I can do. πŸ˜„

nvuillam commented 3 years ago

I'm checking your log to try to reproduce the issue Before that, I suggest you add in your .megalinter.yml config file :

DISABLE:
  - EDITORCONFIG
ADDITIONAL_EXCLUDED_DIRECTORIES:
  - "dist"
  - ".jekyll-cache" # I added this one in the default list in my dev branch
joe-sharp commented 3 years ago

Ahh, thank you! Yeah probably would be wise to ignore those completely rather than just with the linters that ran on them during my initial testing.

nvuillam commented 3 years ago

image

I don't see .github/linters in your log... maybe it is related to the "sub-module" ? If it is symlinks maybe the python browsing misses it ....

nvuillam commented 3 years ago

If I manage to provide you remote config files management, will you be ok to close this issue ? 🀣

joe-sharp commented 3 years ago

It is very possible it is related to it being a submodule. I originally didn't think so because docker shouldn't need to pull anything if the directory was mapped into docker. As far as the file system is concerned, it is just a normal directory with config files in it.

Personally I would be fine closing it, but you may want to consider how it works if I provide a directory to test that isn't project root. I think even in fairly normal use cases it would be wise to support mapping a directory or files for this purpose.

joe-sharp commented 3 years ago

I am also willing to try to fix this on my own, with or without the remote config feature just cause it will be fun. πŸ˜„

nvuillam commented 3 years ago

Please be my guest, PRs are very welcome ;) To investigate, you may override LINTER_RULES_PATH with some_folder , and copy your config files in some_folder If it works , it means that the sub-module is the cause of the issue

nvuillam commented 3 years ago

And i'll do the remote config, just cause it will be fun πŸ˜ƒ

But also to allow to store pre-made configs that users could create share, they may even be hosted on this repo ^^

joe-sharp commented 3 years ago

Sounds like a good first path to investigate. I am still working at the day job right now, but as soon as I am done for the evening I am going to take a crack at it!

joe-sharp commented 3 years ago

Ok I got I running locally and using the proper configs. I am going to call this an environment issue. I suspect it may have something to do with how I had to switch from a topic branch that implemented the submodule back to main, which had a static directory, then pulled to get the submodule back. Doing this corrupted the directory in some regard but I am not sure to what extent, just that replacing the whole local copy of the repository allows it to play nicely with the submodule.

I am going to investigate overriding the directory and see if my other usecase works correctly now that I have working test data.

joe-sharp commented 3 years ago

Ok testing with the following command:

mega-linter-runner -e 'SHOW_ELAPSED_TIME=true' -e 'DEFAULT_BRANCH=main' -p _includes

resulted in the behavior I was worried about. Additionally it affects both config files stored at project root as well as the default linters directory. On a closely related note, my .mega-linter.yml at project root was also ignored.

I also tried this, but the behavior was the same:

mega-linter-runner -e 'SHOW_ELAPSED_TIME=true' -e 'DEFAULT_BRANCH=main' -e 'LINTER_RULES_PATH=.github/linters' -p _includes/
joe-sharp commented 3 years ago

This of course makes implementing a fix somewhat controversial as there are quite a few options for how to approach this issue.

As I user do I expect?..:

  1. That despite providing the -p flag my config files in LINTER_RULES_PATH are still used.
  2. That despite providing the -p flag my config files in project root are still used.
  3. That despite providing the -p flag my config files in both project root and LINTER_RULES_PATH are used.
  4. Since the -p flag was provided I expect that I need to ensure config files are present there.
  5. Since the -p flag was provided I expect that if config files are needed I provide an additional flag for those.
nvuillam commented 3 years ago
  1. Every config variable is related to -p ? --path tells mega-linter-runner what folder to mount with /tmp/lint of docker image, so all variables containing paths are relative to /tmp/lint in the mega-linter docker image :)

Note: I probably did not take enough time to read correctly as it is the middle of the night here, my answer may be better tomorrow :)

joe-sharp commented 3 years ago

Ha that's ok feel free to reread tomorrow. I think 6 would be fair but would probably still need a fix. Docker would need to be sure and mount a common parent directory to both the argument to the -p flag and the linter rules/config file. You could maybe make this work with just the -p flag if the documentation said to provide the full path from say project root:

mega-linter-runner -e 'SHOW_ELAPSED_TIME=true' -e 'DEFAULT_BRANCH=main' -e 'LINTER_RULES_PATH=.github/linters' -p treehouse-vue/_includes/

Suggesting that in the above example that the LINTER_RULES_PATH is relative from the base directory: treehouse-vue (treehouse-vue/.github/linters)

Then mega-linter-runner would need to make sure docker was mounting the treehouse-vue directory as well, but somehow only linting the contents of _includes

nvuillam commented 3 years ago

@joe-sharp > It's now ok for using remote config files :)

When this job will be completed, It will be available with insiders version of the GitHub Action ( nvuillam/mega-linter@insiders ) , or earlier if you use a flavor

Please can you tell me if it works for you, so I can put it in V4 release ?

nvuillam commented 3 years ago

Then mega-linter-runner would need to make sure docker was mounting the treehouse-vue directory as well, but somehow only linting the contents of _includes

You can define FILTER_REGEX_INCLUDE: (_includes) if you really want all linters to lint only this folder If you want a single language or linter to lint _includes , you can define something like

joe-sharp commented 3 years ago

I'll have to give it a shot in the morning, but I am super excited to try it out! I'll let you know how it goes.

joe-sharp commented 3 years ago

@nvuillam

I got hung up once because I left a trailing slash on the URL. Might want to trim trailing slashes for the user.

This run choked somehow. Strangely enough the pull request portion went fine: https://github.com/joe-sharp/old-arduino-projects/pull/1/checks?check_run_id=1564774807

nvuillam commented 3 years ago

mmmm I have issues understanding the crash ... i may add some big crash handler :/

Thanks for the tip to remove last backslash if existing, i'll handle that :)

About https://github.com/joe-sharp/old-arduino-projects/runs/1564775020?check_suite_focus=true, it passed but only using remove .cspell.json file, maybe i should change log level from error to warning for the files not found ? image

joe-sharp commented 3 years ago

Ahh yeah that might be a good idea. A warn for that would probably be ideal since not everyone needs to change from the default config. I suppose it is a best practice to have them so you can control when they update/change though.

nvuillam commented 3 years ago

That's done but installation of rakudo (perl6) does not work anymore, so i can't release a new version... I'll try to fix it ^^

nvuillam commented 3 years ago

Done and released in @insiders (gha) / latest (docker)

joe-sharp commented 3 years ago

Working great! https://github.com/joe-sharp/old-arduino-projects/actions/runs/437252988

Thanks much for the new feature!

nvuillam commented 3 years ago

I'm glad to hear that :)

I also see that when VALIDATE_ALL_CODEBASE is false , flavours suggestion are calculated anyway and shouldn't be ... I'll correct that ^^