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

fix(pre-commit): corrected pre-commit args for megalinter-full #2411

Merged
merged 4 commits into from
Mar 4, 2023

Conversation

drbothen
Copy link
Contributor

@drbothen drbothen commented Mar 3, 2023

containername argument contained both the flag and value on the same line. These have to be on a separate line

This corrects an error with the use of pre-commit with megalinter-full. An error would return about not having a specified container name.

Proposed Changes

  1. Modified .pre-commit-hooks.yaml megalinter-full args containername to match megalinter-incremental

Readiness Checklist

Author/Contributor

  • Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

containernam argument contained bot the flag and value on the same line. These have to be separate line
@drbothen drbothen marked this pull request as ready for review March 3, 2023 21:31
@drbothen drbothen requested a review from nvuillam as a code owner March 3, 2023 21:31
@Kurt-von-Laven
Copy link
Collaborator

Can you elaborate on how to reproduce this error? We have been using the pre-commit hooks successfully for a while now. You are the second person to report this, but I would like to know how I can meaningfully test this change.

@drbothen
Copy link
Contributor Author

drbothen commented Mar 4, 2023

Can you elaborate on how to reproduce this error? We have been using the pre-commit hooks successfully for a while now. You are the second person to report this, but I would like to know how I can meaningfully test this change.

For sure, I installed it using npm install mega-linter-runner -g on my ubuntu server (Ubuntu 22.04.1 LTS) development environment. My dev environment has dual 64-core AMD EPYC with 1TB of RAM.

my .mega-linter.yaml file is as follows:

# Configuration file for MegaLinter
# See all available variables at https://megalinter.io/configuration/ and in linters documentation

APPLY_FIXES: all # all, none, or list of linter keys
# ENABLE: # If you use ENABLE variable, all other languages/formats/tooling-formats will be disabled by default
# ENABLE_LINTERS: # If you use ENABLE_LINTERS variable, all other linters will be disabled by default
# DISABLE:
# - COPYPASTE # Uncomment to disable checks of excessive copy-pastes
# - SPELL # Uncomment to disable checks of spelling mistakes
SHOW_ELAPSED_TIME: true
# FILEIO_REPORTER: true # Uncomment to upload artifacts to FileIO
# DISABLE_ERRORS: true # Uncomment if you want MegaLinter to detect errors but not block CI to pass
SPELL_CSPELL_DISABLE_ERRORS: true # Uncomment if you want MegaLinter to detect spelling mistakes but not block CI to pass

my pre-commit is configured using the following:

exclude: "^docs/|/migrations/"
default_stages: [commit]

repos:
  - repo: https://github.com/asottile/pyupgrade
    rev: v3.3.1
    hooks:
      - id: pyupgrade
        args: [--py310-plus]

  - repo: https://github.com/commitizen-tools/commitizen
    rev: v2.42.0
    hooks:
      - id: commitizen

  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.4.0
    hooks:
      - id: check-added-large-files
      - id: check-case-conflict
      - id: check-merge-conflict
      - id: check-symlinks
      - id: destroyed-symlinks
      - id: detect-private-key
      - id: check-json
      - id: check-yaml
      - id: no-commit-to-branch
        args: [--branch, main, --branch, develop]
      - id: end-of-file-fixer
      - id: forbid-new-submodules
      - id: trailing-whitespace

  - repo: https://github.com/oxsecurity/megalinter
    rev: v6.19.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

# sets up .pre-commit-ci.yaml to ensure pre-commit dependencies stay up to date
ci:
  autoupdate_schedule: weekly
  skip: []
  submodules: false

The above config will generate the below error when magalinter-full is triggered

[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to /home/jmagady.fam@ad.recitsols.com/.cache/pre-commit/patch1677900863-182882.
pyupgrade............................................(no files to check)Skipped
check for added large files..............................................Passed
check for case conflicts.................................................Passed
check for merge conflicts................................................Passed
check for broken symlinks............................(no files to check)Skipped
detect destroyed symlinks................................................Passed
detect private key.......................................................Passed
check json...........................................(no files to check)Skipped
check yaml...............................................................Passed
don't commit to branch...................................................Passed
fix end of files.........................................................Passed
forbid new submodules................................(no files to check)Skipped
trim trailing whitespace.................................................Passed
Run MegaLinter...........................................................Failed
- hook id: megalinter-full
- exit code: 1

/usr/lib/node_modules/mega-linter-runner/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 (/usr/lib/node_modules/mega-linter-runner/node_modules/optionator/lib/index.js:316:17)
    at Object.parse (/usr/lib/node_modules/mega-linter-runner/node_modules/optionator/lib/index.js:356:13)
    at MegaLinterRunnerCli.run (/usr/lib/node_modules/mega-linter-runner/lib/cli.js:10:39)
    at /usr/lib/node_modules/mega-linter-runner/lib/index.js:13:37
    at Object.<anonymous> (/usr/lib/node_modules/mega-linter-runner/lib/index.js:14:5)
    at Module._compile (node:internal/modules/cjs/loader:1226:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1280:10)
    at Module.load (node:internal/modules/cjs/loader:1089:32)
    at Module._load (node:internal/modules/cjs/loader:930:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)

Node.js v18.14.0

if I change the pre-commit config to the following, the error goes away:

exclude: "^docs/|/migrations/"
default_stages: [commit]

repos:
  - repo: https://github.com/asottile/pyupgrade
    rev: v3.3.1
    hooks:
      - id: pyupgrade
        args: [--py310-plus]

  - repo: https://github.com/commitizen-tools/commitizen
    rev: v2.42.0
    hooks:
      - id: commitizen

  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.4.0
    hooks:
      - id: check-added-large-files
      - id: check-case-conflict
      - id: check-merge-conflict
      - id: check-symlinks
      - id: destroyed-symlinks
      - id: detect-private-key
      - id: check-json
      - id: check-yaml
      - id: no-commit-to-branch
        args: [--branch, main, --branch, develop]
      - id: end-of-file-fixer
      - id: forbid-new-submodules
      - id: trailing-whitespace

  - repo: https://github.com/oxsecurity/megalinter
    rev: v6.19.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
        # below required until PR #2411 is merged https://github.com/oxsecurity/megalinter/pull/2411
        args:
          - mega-linter-runner
          - --containername
          - "megalinter-full"
          - --remove-container
          - --fix
          - --env
          - "'APPLY_FIXES=all'"
          - --env
          - "'CLEAR_REPORT_FOLDER=true'"
          - --env
          - "'LOG_LEVEL=warning'"
        stages:
          - push

# sets up .pre-commit-ci.yaml to ensure pre-commit dependencies stay up to date
ci:
  autoupdate_schedule: weekly
  skip: []
  submodules: false

In the code on megaliter-incremental, you define the arg flags and values on separate lines. I replicated this behavior for magalinter-full and it resolved the error.

Below is the current .pre-commit-hooks.yaml in megalinter

- id: megalinter-incremental
  name: Run MegaLinter (skipping linters that run in project mode)
  entry: npx --
  language: system
  require_serial: true
  args:
    - mega-linter-runner
    - --containername
    - "megalinter-incremental"
    - --remove-container
    - --fix
    - --env
    - "'APPLY_FIXES=all'"
    - --env
    - "'CLEAR_REPORT_FOLDER=true'"
    - --env
    - "'LOG_LEVEL=warning'"
    - --filesonly
  description: >
    See https://megalinter.io/latest/mega-linter-runner/#usage
    and https://megalinter.io/latest/configuration/ if you
    wish to override the default arguments. mega-linter-runner is specified as
    an argument so that you may override the version (e.g.,
    mega-linter-runner@vx.y.z). Depends on npx, which ships with npm 7+, and
    Docker. Runs very slowly when the pertinent Docker image isn't already
    cached (c.f., https://github.com/marketplace/actions/docker-cache/). If you
    encounter permission errors, try running Docker in rootless mode (c.f.,
    https://github.com/marketplace/actions/rootless-docker/). Linter results are
    logged to the megalinter-reports directory, so list it in your .gitignore.
    Skip linters that run in project mode since they don't run incrementally.
- id: megalinter-full
  name: Run MegaLinter
  entry: npx --
  language: system
  require_serial: true
  args:
    - mega-linter-runner
    - --containername megalinter-full <--- This is causing the error, --containname and megalinter-full need to be on seperate lines (see above)
    - --remove-container
    - --fix
    - --env
    - "'APPLY_FIXES=all'"
    - --env
    - "'CLEAR_REPORT_FOLDER=true'"
    - --env
    - "'LOG_LEVEL=warning'"
  description: >
    See https://megalinter.io/latest/mega-linter-runner/#usage
    and https://megalinter.io/latest/configuration/ if you
    wish to override the default arguments. mega-linter-runner is specified as
    an argument so that you may override the version (e.g.,
    mega-linter-runner@vx.y.z). Depends on npx, which ships with npm 7+, and
    Docker. Runs very slowly when the pertinent Docker image isn't already
    cached (c.f., https://github.com/marketplace/actions/docker-cache/). If you
    encounter permission errors, try running Docker in rootless mode (c.f.,
    https://github.com/marketplace/actions/rootless-docker/). Linter results are
    logged to the megalinter-reports directory, so list it in your .gitignore.
    

To test, just create a new repository, install pre-commit package, dump my pre-commit config in, change push to commit and comment out the incremental. dump a test python file in, then run pre-commit. You will see the error.

Does this help?

Copy link
Collaborator

@Kurt-von-Laven Kurt-von-Laven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for catching this. I was able to reproduce this issue following the steps you provided!

@@ -36,7 +36,8 @@
require_serial: true
args:
- mega-linter-runner
- --containername megalinter-full
- --containername
- "megalinter-full"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the double quotes actually necessary?

Copy link
Member

@nvuillam nvuillam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching the issue and fixing it :)

@nvuillam nvuillam merged commit c4744b6 into oxsecurity:main Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants