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

ENABLE_ERRORS_LINTERS #3599

Open
wesley-dean-flexion opened this issue May 31, 2024 · 16 comments
Open

ENABLE_ERRORS_LINTERS #3599

wesley-dean-flexion opened this issue May 31, 2024 · 16 comments
Labels
enhancement New feature or request

Comments

@wesley-dean-flexion
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Right now, we have DISABLE_ERRORS_LINTERS as a convenient way to provide a list of linters whose findings will not cause MegaLinter to fail a build.

The vision I have is that MegaLinter could run and fail builds if scanners in REPOSITORY_.* produce findings but allow MegaLinter to pass builds that have findings in the other linters.

So, the security-centric REPOSITORY_.* scanners findings' will be errors while everything else has errors disabled. It's the inverse of DISABLE_ERRORS_LINTERS.

Describe the solution you'd like

Example:

DISABLE_ERRORS: true
ENABLE_ERRORS_LINTERS:
  - REPOSITORY-SEMGREP
  - REPOSITORY-GITLEAKS
  - [.. rest of the security scanners go here]

(the first line may or may not be implied by the presence of ENABLE_ERRORS_LINTERS)

Describe alternatives you've considered

DISABLE_ERRORS: false
DISABLE_ERRORS_LINTERS:
  - MARKDOWN_MARKDOWNLINT
  - MARKDOWN_REMARK_LINT
  - MARKDOWN_MARKDOWN_LINK_CHECK
  - MARKDOWN_MARKDOWN_TABLE_FORMATTER
  - [.. every other linter MegaLinter supports except the security scanners]

Additional context

This could mirror the behavior of other variables in the activation / deactivation functionality, but that may be more involved than would be required at an early stage.

I looked through the code and the issues on GitHub but I didn't see what I was looking for, but that's not to say there isn't a fancy, super-excellent way to get from here to there without changing a line of code.

Just an idea. Thank you!

@wesley-dean-flexion wesley-dean-flexion added the enhancement New feature or request label May 31, 2024
@nvuillam
Copy link
Member

nvuillam commented Jun 2, 2024

@wesley-dean-flexion good idea :)
If you make a PR allowing that, + update the documentation, I'll gladly accept it :)

@wesley-dean-flexion
Copy link
Contributor Author

@nvuillam Sure. I'm thinking that when it's invoked, we handle it as the user specified DISABLE_ERRORS_LINTERS and, instead of setting it to the value the user provided, set it to the difference of what the user provided:

    self.disable_errors_linters = list_all_linters().difference (
        config.get_list(
            self.request_id, "DISABLE_ERRORS_LINTERS", self.disable_errors_linters
        )
    )

I haven't tested that to see if it would work -- I don't even know if the types would align -- but I think that ought to be sufficient to convey the idea.

Are my logic and reasoning sound?

@nvuillam
Copy link
Member

nvuillam commented Jun 20, 2024

@wesley-dean-flexion like for ENABLE_LINTERS and DISABLE_LINTERS, if ENABLE variable is set, then its value is applied , and DISABLE variable is ignored :)

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 Jul 21, 2024
@wesley-dean-flexion
Copy link
Contributor Author

I'll get on it... I swear.......

@nvuillam
Copy link
Member

@wesley-dean-flexion you do when you can , that's ok :)

Note: next major version should happen in the following weeks :)

@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 Jul 23, 2024
@wesley-dean-flexion
Copy link
Contributor Author

@nvuillam Outstanding about the new major version!

@wesley-dean-flexion
Copy link
Contributor Author

Hey @nvuillam. I thunk another thought this morning. This is fresh and I haven't verified anything yet, so I could be wildly incorrect with anything and everything I'm about to say.

Currently, DISABLE_ERRORS, when True, results in no errors, warnings, etc. result in MegaLinter returning a non-zero result when run. It always succeeds and returns a 0 when the process finishes.

Suppose DISABLE_ERRORS wasn't so much an absolute "You! Shall not! Fail!!!" but was, instead, the default value for all of the (descriptor)_(linter)_DISABLE_ERRORS values. For example, suppose I had a Python program and there was an error pylint was catching. It's an error, so it would typically result in a non-zero code being returned which MegaLinter would detect.

Suppose this is an excerpt from some project's .mega-linter.yml file:

DISABLE_ERRORS: True
PYTHON_PYLINT_DISABLE_ERRORS: False

Currently, this case would cause MegaLinter to return a 0 (success) because DISABLE_ERRORS is set to True.

HOWEVER, if DISABLE_ERRORS isn't absolute but, instead, the default value for linters' _DISABLE_ERRORS values, then when pylint returns non-zero, because PYTHON_PYLINT_DISABLE_ERRORS is set to False, then MegaLinter would also return non-zero. If black, however, were to return non-zero, because DISABLE_ERRORS is True and PYTHON_BLACK_DISABLE_ERRORS is undefined, the DISABLE_ERRORS (default) becomes the effective value and a zero result code (success) would be returned.

So, if I wanted to only allow pylint to cause a MegaLinter run to fail, I would use the above configuration. If black was non-zero, MegaLinter would still respond with a zero.

This is different from the current interpretation of DISABLE_ERRORS in that if DISABLE_ERRORS was set to True, the pylint error would cause MegaLinter to respond with zero; if DISABLE_ERRORS were set to False, the black error would cause MegaLinter to respond with non-zero.

I think this would be simpler and less ambiguous than the initial ENABLE_ERRORS_LINTERS idea.

What do you think?

@nvuillam
Copy link
Member

@wesley-dean-flexion in my opinion errors should not be disabled by default, and it's only during the setup and after checking the blocking errors, that someone can decide to consider some linters unrelevant , and individually disable them

But as it seems that your update would not change the current behaviour (that would induce an unwanted breaking change), I'd be ok to validate a PR that "forces enablement of linter errors" if DISABLE_ERRORS is defined to true :)

@wesley-dean-flexion
Copy link
Contributor Author

wesley-dean-flexion commented Aug 15, 2024

errors should not be disabled by default

Agreed. I generally find acceptable that errors cause a MegaLinter run to fail while warnings do not.

would not change the current behavior

Also agreed. My hope and desire is to not change any existing, default behavior. The only way this new functionality would happen is when DISABLE_ERRORS is set to True and individual linters' _DISABLE_ERRORS values were explicitly set to False.

  • DISABLE_ERRORS is False or undefined (default)

    • _DISABLE_ERRORS is True: errors from this linter produce zero result (same as now)
    • _DISABLE_ERRORS is False or undefined: errors from this linter result produce non-zero result (same as now)
  • DISABLE_ERRORS is True

    • _DISABLE_ERRORS is True or undefined: errors from this linter produce zero result (same as now)
    • _DISABLE_ERRORS is False: errors from this linter produce non-zero result (THIS IS NEW)

@wesley-dean-flexion
Copy link
Contributor Author

wesley-dean-flexion commented Aug 15, 2024

The goal is still the same as before: I would like to be able to specify a list of linters where errors affirmatively cause MegaLinter to return a non-zero result code while the other linters do not.

It would allow something like this:

---

DISABLE_ERRORS: True
REPOSITORY_TRIVY_DISABLE_ERRORS: False
REPOSITORY_GRYPE_DISABLE_ERRORS: False

In that situation, errors detected by any linter, scanner, etc. that MegaLinter provides will show up as warnings except for Grype and Trivy. Errors found by those tools will cause MegaLinter to return a non-zero result code.

My vision is that the security-related scanners (so, mostly the REPOSITORY descriptor) can cause MegaLinter to serve as a quality gate that would fail a PR-related merge. If someone wants to put one or two spaces between content and a comment in a YAML file, that shouldn't break a build. Having someone leave an RDS instance open to 0.0.0.0 should definitely break a build.

That is, MegaLinter as a security tool first and foremost.

@nvuillam
Copy link
Member

errors should not be disabled by default

Agreed. I generally find acceptable that errors cause a MegaLinter run to fail while warnings do not.

would not change the current behavior

Also agreed. My hope and desire is to not change any existing, default behavior. The only way this new functionality would happen is when DISABLE_ERRORS is set to True and individual linters' _DISABLE_ERRORS values were explicitly set to False.

  • DISABLE_ERRORS is False or undefined (default)

    • _DISABLE_ERRORS is True: errors from this linter produce zero result (same as now)
    • _DISABLE_ERRORS is False or undefined: errors from this linter result produce non-zero result (same as now)
  • DISABLE_ERRORS is True

    • _DISABLE_ERRORS is True or undefined: errors from this linter produce zero result (same as now)
    • _DISABLE_ERRORS is False: errors from this linter produce non-zero result (THIS IS NEW)

works for me :)

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 Sep 15, 2024
@wesley-dean-flexion
Copy link
Contributor Author

activity

@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 Sep 17, 2024
@wesley-dean-flexion
Copy link
Contributor Author

even more activity

@wesley-dean-flexion
Copy link
Contributor Author

leaving myself a breadcrumb: https://github.com/oxsecurity/megalinter/blob/main/megalinter/Linter.py#L746

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants