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.81k stars 215 forks source link

Pre-commit hook integration is broken #3061

Open vorburger opened 8 months ago

vorburger commented 8 months ago

Describe the bug It appears https://megalinter.io/latest/mega-linter-runner/#pre-commit-hook is broken?

To Reproduce

Add https://megalinter.io/latest/mega-linter-runner/#pre-commit-hook to https://github.com/www-learn-study/saraswati.learn.study/blob/main/.pre-commit-config.yaml,

which uses pre-commit==3.5.0 (see https://github.com/www-learn-study/saraswati.learn.study/blob/main/requirements.txt)

run https://github.com/www-learn-study/saraswati.learn.study/blob/main/bin/pre-commit.sh

Expected behavior It works.

Actual behavior

$ pre-commit run

Detect hardcoded secrets.........................................................Passed
fix end of files.................................................................Passed
trim trailing whitespace.........................................................Passed
Run MegaLinter (skipping linters that run in project mode).......................Failed
- hook id: megalinter-incremental
- exit code: 1

/home/vorburger/.npm/_npx/614e542633a698fb/node_modules/optionator/lib/index.js:316
          throw new Error("Value for '" + prop + "' of type '" + getOption(prop).type + "' required.");
                ^

Error: Value for 'container-name' of type 'String' required.
    at checkProp (/home/vorburger/.npm/_npx/614e542633a698fb/node_modules/optionator/lib/index.js:316:17)
    at Object.parse (/home/vorburger/.npm/_npx/614e542633a698fb/node_modules/optionator/lib/index.js:356:13)
    at MegaLinterRunnerCli.run (file:///home/vorburger/.npm/_npx/614e542633a698fb/node_modules/mega-linter-runner/lib/cli.js:8:39)
    at file:///home/vorburger/.npm/_npx/614e542633a698fb/node_modules/mega-linter-runner/lib/index.js:12:37
    at file:///home/vorburger/.npm/_npx/614e542633a698fb/node_modules/mega-linter-runner/lib/index.js:13:5
    at ModuleJob.run (node:internal/modules/esm/module_job:194:25)

Node.js v18.18.2

Additional context

Perhaps there is a way for the project to detect this earlier by somehow self testing this?

Kurt-von-Laven commented 8 months ago

Thank you for reporting this issue. As a workaround, I would suggest passing the --container-name as an argument (an args array) to the pre-commit hook. Pre-commit has the unfortunate restriction of requiring hooks to be pinned to a release, making self-testing surprisingly cumbersome. I am open to suggestions from the community about how to achieve this or PRs.

nvuillam commented 8 months ago

Couldn't we use a default container name if not sent in options ?

Kurt-von-Laven commented 8 months ago

Yes, that is a very easy fix.

github-actions[bot] commented 7 months ago

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.

nvuillam commented 7 months ago

Any idea if this bug is still to solve ?

lewis-jackson-bots commented 2 months ago

@nvuillam I don't think that the pre-commit hooks as defined can be customised due to the use of args. e.g. if I have this pre-commit config:

  - repo: https://github.com/oxsecurity/megalinter
    rev: v7.10.0 # Git tag specifying the hook, not mega-linter-runner, version
    hooks:
      - id: megalinter-incremental # Faster, less thorough
        stages:
          - commit
      - id: megalinter-full # Slower, more thorough
        stages:
          - push
        args:
          - --containername
          - terraform

The args will just be passed to npx -- like npx -- containername terraform overriding the defaults on this repo.

Let me know if I'm not doing this correctly!

For now I've just defined a local job:

  - repo: local
    hooks:
      - id: megalinter-terraform
        name: megalinter-terraform
        language: system
        require_serial: true
        entry: npx --
        args:
          - mega-linter-runner
          - --containername
          - megalinter-terraform
          - --remove-container
          # - --fix
          # - --env
          # - "'APPLY_FIXES=all'"
          - --env
          - "'CLEAR_REPORT_FOLDER=true'"
          - --env
          - "'LOG_LEVEL=warning'"
nvuillam commented 2 months ago

Our pre-commit expert is @Kurt-von-Laven ... do you have any idea Kurt ? :)

lewis-jackson-bots commented 2 months ago

Args you don't want to be overwritten need to be dumped into the entry