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

Mega Linter and pre-commit hooks #569

Closed
milica-nikolic opened this issue Jul 14, 2021 · 27 comments · Fixed by #1438
Closed

Mega Linter and pre-commit hooks #569

milica-nikolic opened this issue Jul 14, 2021 · 27 comments · Fixed by #1438
Assignees
Labels
nostale This issue or pull request is not stale, keep it open question Further information is requested

Comments

@milica-nikolic
Copy link

Hi everyone!

I'm curious is there a way to configure mega-linter so it can be combined with git hooks for pre-commits ? I would like to lint only updated files before commits and after that mega-linter would lint whole project while he is implemented in CI tool ?

Thank you in advance ! :)

@milica-nikolic milica-nikolic added the question Further information is requested label Jul 14, 2021
@nvuillam
Copy link
Member

nvuillam commented Jul 14, 2021

Mega-Linter relies on a docker image, so it may be heavy to do a docker run every time you want to commit, it's probably best to let CI servers do that for you :)

But if you really want to do that... I could consider creating a pre-commit hook based on mega-linter-runner :) Or maybe you'd like to do so ?

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Aug 14, 2021
@tpansino
Copy link
Contributor

@milica-nikolic , here's an example .pre-commit-config.yaml I use to run the docker image via pre-commit:

repos:
 - repo: local
    hooks:
      - id: mega-linter
        name: Mega-Linter
        language: docker_image
        pass_filenames: false
        entry: "-u root -e DEFAULT_WORKSPACE=/src nvuillam/mega-linter:v4.34.0"

Notes:

  • I also include REPORT_OUTPUT_FOLDER: /src/.mega-linter in my .mega-linter.yml config. This combined with the -e DEFAULT_WORKSPACE=/src makes it so you can view the linter reports outside of the Docker container, after pre-commit finishes. Be sure to also .gitignore the .mega-linter folder though, or else MegaLinter tries to lint the code samples in the reports. 😄
  • The container version is set manually in the entry:, whish is what I recommend instead of using latest, Docker doesn't pull from latest automatically on updates, because Docker thinks latest is just an ordinary tag, and sees it in the cache. Setting an explicit version busts the cache and makes Docker update the image, transparent to users.

@nvuillam I planned to write these instructions up someday and make them part of the docs, just haven't had time yet 😄

@github-actions github-actions bot removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Aug 20, 2021
@nvuillam
Copy link
Member

That's great news, thanks :)

@milica-nikolic
Copy link
Author

I had different approach. I have used dotnet format tool in pre commit script so I can fix errors that mega-linter reported in GitLab CI pipeline.

Your approach is way better and I will look deeper into that.

You mentioned that you plan to write instructions, please link them when you finish them. I am sure that they will be quite helpful.

Thank you for help! 😃

@Kurt-von-Laven
Copy link
Collaborator

Kurt-von-Laven commented Sep 19, 2021

It would be awesome to add a .pre-commit-hooks.yaml to MegaLinter so that downstream projects can merely reference this repo and the MegaLinter hook id without having to be as clever as @tpansino. @nvuillam, how close is the excerpt above to what would be needed?

@nvuillam
Copy link
Member

nvuillam commented Sep 19, 2021

I'm not a pre-commit user (I like my commits to just commit :p ), so I'm not familiar with this tool ....

I'm also afraid that running a docker image during a pre-commit hook may be not very user friendly ... but for those who really want to do it, @tpansino maybe you could implement this famous .pre-commit-hooks.yaml ? 👼 or @Kurt-von-Laven ? :)

@Kurt-von-Laven
Copy link
Collaborator

Kurt-von-Laven commented Sep 20, 2021

I looked through pre-commit's instructions for running an existing docker_image and MegaLinter Runner's command line options, and believe the following .pre-commit-hooks.yaml would be a good start. Future work could include supporting VALIDATE_ALL_CODEBASE: false and running without root, in which case files could be passed directly to MegaLinter by pre-commit since MegaLinter presently crashes trying to work out which files to lint on its own. Many people may prefer running MegaLinter from a pre-commit hook to running 10 separate linters directly and getting the same result but more slowly. One would need some mechanism by which to ensure that the version of the Docker image stays in sync with the version of the GitHub repository itself. I did not encounter the issue reported by @tpansino where the linter reports aren't accessible outside of the Docker container.

-   id: megalinter
    name: Run MegaLinter
    entry: >
      --user root
      --env DEFAULT_WORKSPACE=/src
      --env FORMATTERS_DISABLE_ERRORS=false
      --env LOG_LEVEL=warning
      --env VALIDATE_ALL_CODEBASE=true
      megalinter/megalinter:<version>
    language: docker_image
    pass_filenames: false
    description: Run MegaLinter on changed files.
    args: [--fix]

@Kurt-von-Laven
Copy link
Collaborator

@nvuillam, is there some mechanism by which one could run Mega-Linter only on the files passed to it? One of pre-commit's big selling points is how much faster it runs than traditional CI since most linters only need to consider the staged files that it passes to them by default. Even on a small code base with a specific flavor, commits take multiple minutes. Also, do you happen to know off the top of your head why root is needed? Not sure if that is a consequence of the way pre-commit runs Docker containers.

@nvuillam
Copy link
Member

nvuillam commented Sep 30, 2021

Hmmmm there is not, but it can probably be done by sending the list of files as argument :)

At the moment Mega-Linter collects files, we could do something like:

if MEGALINTER_FILES_TO_LINT env var is set
files_to_lint = MEGALINTER_FILES_TO_LINT
else
usual way

Could be adapted within an --files argument for mega-linter-runner too

About root, no idea... it was like that on Super-Linter ^^

@Kurt-von-Laven
Copy link
Collaborator

I opened a separate feature request to support linting passed files based on this discussion in case someone can beat me to it.

About root, no idea... it was like that on Super-Linter ^^

I believe I figured this part out however. We recently released rootless-docker, a simple GitHub Action that discusses what I believe is going on here.

One would need some mechanism by which to ensure that the version of the Docker image stays in sync with the version of the GitHub repository itself.

As for this piece, the Python-based Commitizen takes a version_files argument, so I believe one could wire up their bump-version Action, and specify .pre-commit-hooks.yaml as a version file.

@Kurt-von-Laven
Copy link
Collaborator

Kurt-von-Laven commented Oct 14, 2021

@nvuillam it dawned on me that the issue preventing incremental runs was trivially fixed and actually related to the fact that I was rebasing, not so much the fact that I was using pre-commit (though of course there is an expectation that pre-commit hooks not crash if run while rebasing). See #851.

Also, I found a better way of writing the pre-commit hook. I will open a pull request if this seems reasonable to you, because by the time the hook is released, I doubt we will still need to require that VALIDATE_ALL_CODEBASE=true on account of the aforementioned fix and #760.

- id: megalinter
  name: Run JavaScript flavor of MegaLinter
  entry: mega-linter-runner
  language: node
  pass_filenames: false
  args:
    - --flavor
    - javascript
    - --env
    - LOG_LEVEL=warning
    - --env
    - VALIDATE_ALL_CODEBASE=true
    - --release
    - v4.46.0
    - --fix
  stages:
    - push
  additional_dependencies:
    - mega-linter-runner@v4.46.0

@tpansino
Copy link
Contributor

tpansino commented Oct 16, 2021

Apologies, I've been really busy and am catching up on this thread.

@Kurt-von-Laven, the example in your previous message is a proposed .pre-commit-hooks.yaml, yes? Have you actually tested it? I just forked and tried it and it's not working for me.

I too tried implementing a pre-commit hook this way back in March, until I encountered pre-commit/pre-commit#1858. Basically for Node execution, pre-commit expects a package.json to be in the top-level folder of the repo, and Mega Linter's is in the mega-linter-runner sub-folder. The maintainer doesn't want to support any customization there as you can see from the discussion, so if you've found a way around that limitation - great!

Otherwise, I meant to eventually ask @nvuillam what it would take to split the mega-linter-runner into it's own repo? Or perhaps we could just create a shim repo that meets the interface requirements for pre-commit to be able to install and run Mega Linter?

FWIW - I'm using Mega-Linter extensively these days via Pre-commit, and with this (deprecated 😞) GitHub Action to enable a consistent UX between local and CI/CD pipeline. It allows for maintaining a single config for linting enforcement on the CI/CD, but faster feedback for users that install the pre-commit hook if they want. I love it, and I'm happy to help test any pre-commit integration out to better enable the pattern ❤️

@Kurt-von-Laven
Copy link
Collaborator

Kurt-von-Laven commented Oct 17, 2021

@tpansino, I was being rather vague, but that is a way of creating a local hook now in your (.pre-commit-config.yaml) that works for us by installing and running the mega-linter-runner Node.js package. There is some funny business going on related to config files as mentioned above, but our .NET flavor hook worked fully out of the box. I do wish that pre-commit could install a non-local hook from npm rather than having to git clone everything.

Yeah, there are a lot of mirror repos out there that serve the purpose of stuffing a pre-commit hook into a project that doesn't natively support it, and I assume that is generally how monorepos are handled. I was spacing out about this previously, so I am glad you brought it up. I think it's simply a matter of whether @nvuillam sees value in splitting the repo.

Incidentally, there is a central repository of hooks.

Ha ha, our linting stacks are the same, and I also love it. It's especially awesome in tandem with asdf to be able to use the same toolchain regardless of language and OS (via WSL2) and Poetry to pin pre-commit’s dependencies.

@nvuillam
Copy link
Member

nvuillam commented Oct 17, 2021

@Kurt-von-Laven @tpansino

As I have automation that updates source of both mega-linter and mega-linter-runner, I'd rather use the mirror repos solution :)

https://github.com/nvuillam/pre-commit-hooks-mega-linter is all yours, waiting for PR ^^

About the version used, you can use v4, it always contains the latest v4.x.x release, so no maintenance will be needed when there will be new Mega-Linter releases :)

@tpansino
Copy link
Contributor

@Kurt-von-Laven if you've got the time to put something together, I'm happy to review. Otherwise it's going on my backlog 😆

CC @TimPansino - this effort might interest you.

@Kurt-von-Laven
Copy link
Collaborator

I'll take a stab at mirroring mega-linter-runner at some point in the next few weeks. I realized that I somehow must have changed something related to our yamllint config, so I removed my caveat about that to avoid discouraging people from using a perfectly good pre-commit hook in the meantime.

@Kurt-von-Laven
Copy link
Collaborator

Kurt-von-Laven commented Nov 5, 2021

I have tested this out for a bit now, and indeed incremental mode is supported at v5. Here is a sample config that demonstrates how you would disable a slow-running linter such as jscpd, which necessarily must always run on all files since it detects duplication, on commit but not push.

      - id: megalinter-dotnet-incremental
        name: Run .NET flavor of MegaLinter (skipping jscpd)
        entry: megalinter-runner
        language: node
        pass_filenames: false
        args:
          - --flavor
          - dotnet
          - --env
          - LOG_LEVEL=warning
          - --env
          - VALIDATE_ALL_CODEBASE=false
          - --env
          - DISABLE_LINTERS=COPYPASTE_JSCPD
          - --release
          - v5
          - --fix
        stages:
          - commit
        additional_dependencies:
          - mega-linter-runner@v5
      - id: megalinter-dotnet
        name: Run .NET flavor of MegaLinter
        entry: megalinter-runner
        language: node
        pass_filenames: false
        args:
          - --flavor
          - dotnet
          - --env
          - LOG_LEVEL=warning
          - --env
          - VALIDATE_ALL_CODEBASE=true
          - --release
          - v5
          - --fix
        stages:
          - push
        additional_dependencies:
          - mega-linter-runner@v5

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Dec 15, 2021
@nvuillam nvuillam removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Jan 18, 2022
@schorlton
Copy link
Contributor

Very similar to this, you can set up a precommit hook with:

docker run --rm -v "$PWD":/tmp/lint \
    -e "VALIDATE_ALL_CODEBASE=false" \
    megalinter/megalinter:v5

However, @nvuillam, is there a way to set megalinter to only lint staged files? Commonly devs work with dirty repos and don't want to lint all of the changed files, as is done (if I understand correctly) with VALIDATE_ALL_CODEBASE=false. Thanks!

@nvuillam
Copy link
Member

@schorlton this is an evolution we have planned yes :) (being able to send a list of files to MegaLinter)

@Kurt-von-Laven
Copy link
Collaborator

@schorlton, if you wouldn't mind giving #808 a thumbs up, that will help us keep track of what feature requests are in highest demand.

@schorlton
Copy link
Contributor

@schorlton, if you wouldn't mind giving #808 a thumbs up, that will help us keep track of what feature requests are in highest demand.

Done, thanks!

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Feb 26, 2022
@Kurt-von-Laven Kurt-von-Laven removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Mar 10, 2022
@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Apr 10, 2022
@Kurt-von-Laven Kurt-von-Laven added nostale This issue or pull request is not stale, keep it open and removed O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity labels Apr 11, 2022
@Kurt-von-Laven
Copy link
Collaborator

I believe I have finally cracked this obstinate nut. See these hooks. They are tailored to my team's specific needs, but I will contribute a more general version of these to the official MegaLinter repo if folks like what they see.

@Kurt-von-Laven
Copy link
Collaborator

Learned a few lessons from using these pre-commit hooks internally, and incorporated them into the linked PR. Hope people find them helpful! Some of the folks on this thread may be interested in some of my company's open-source projects since they relate to GitHub Actions, pre-commit, Docker, asdf, and of course MegaLinter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nostale This issue or pull request is not stale, keep it open question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants