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://ox.security
GNU Affero General Public License v3.0
1.97k stars 239 forks source link

Support Auto-Fix with Sqlfluff #2784

Open BrunoJuchli opened 1 year ago

BrunoJuchli commented 1 year ago

Problem

Currently megalinter Sqlfluff integration does not support fixing of files.

I have tried different combinations, like:

passing fix as argument

SQL_SQLFLUFF_ARGUMENTS: "fix"
SQL_SQLFLUFF_CLI_LINT_MODE: "file"

=> Error says fix is not a valid file path

reconfiguring executable

SQL_SQLFLUFF_CLI_EXECUTABLE: ['sqlfluff', 'fix']
SQL_SQLFLUFF_CLI_LINT_MODE: "file"

=> Error says 'lint' is not a valid file path

User Error: Specified path does not exist. Check it/they exist(s): lint.

Proposed Solution

Use the standard way of applying auto-fixes, by either:

-adding 'SQL_SQLFLUFF ' to 'APPLY_FIXES' -setting 'APPLY_FIXES' to 'all'

When auto-fixing is enabled, the following should happen:

Alternative Configurability

Add a new configurable variable

SQL_SQLFLUFF_FIX with a default value of 'false'.

nvuillam commented 1 year ago

@BrunoJuchli thanks for the analysis and propositions :)

When sqlfluff is called with sqlfluff fix, does it also lint ?

There are ways to replace lint by fix just in descripto

image

but replacing the cli_lint_mode could be done only in a Python class associates to the linter descriptor, something like in overridden method before_lint_files

if self.apply_fixes is True:
   self.cli_lint_mode = file

image

Would you like to make a PR ? :)

BrunoJuchli commented 1 year ago

@nvuillam Thanks for the input. Sorry it took me so long to respond. Initially I was under the impression that I could also somehow remove the lint argument without actually modifying the mega-linter code. So I waited until I had some time on my hands to try it out... but of course now I know better ^^

re whether sqlfluff fix also lints: I interpret your question to mean "does it list errors it cannot fix". I had to do a search on this, as it isn't obvious to me either :D So finally I found the answer on their slack channel. For fix + lint we should call it as follows:

sqlfluff fix --show-lint-violations

However, not sure if it actually works. At least for a specific scenario it seems to fail to report linting errors: https://github.com/sqlfluff/sqlfluff/issues/4871

Re providing a PR: last time I worked with python was more than 10 years ago... so I fear it might take me quite long => I don't want my boss to get out his Kloppe ;-)

image

github-actions[bot] commented 1 year 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 1 year ago

@BrunoJuchli I learnt python to build MegaLinter, the code is simple :p

And i'll help if necessary :)

P107RP commented 1 year ago

@nvuillam I am unsure if it is related to the reported problem, but sqlfluff fix is not working for mega-linter-runner either. I tested the following: mega-linter-runner -r v7 --fix mega-linter-runner -r v7 --flavor cupcake --fix and mega-linter-runner -r v7 -e 'APPLY_FIXES=all' Errors are reported correctly however using --fix parameters is not fixing any of SQL errors. If I run locally sqlfluff fix some are fixed. Have you noticed similar issues?

nvuillam commented 1 year ago

@P107RP today sqlfluff fix is not called, it would require a PR ^^

P107RP commented 1 year ago

@nvuillam I think it is a very handy feature ;) Do you have any plans to implement this, with a next version maybe?

nvuillam commented 1 year ago

I have a crazy backlog, not only on MegaLinter, so i'll probably do it someday but that might be not soon ^^

If someone submits a PR I'll be glad to accept it :)

github-actions[bot] commented 1 year 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.

YElyousfi commented 10 months ago

The issue went stale but any chance this will get implemented soon?

nvuillam commented 10 months ago

It depends on if a generous contributor wants to spent some time on it to submit a PR :) On my side... I'm drowning sorry ^^