MegaLinter config brush ups

[eitsupi]

We have set up a CI workflow of MegaLinter in #1974, but the current setup is very rough.

• Auto fix is turned off.
• Configured not to generate errors (users may not notice linter warnings)

Need to follow up based on some future feedback.

A list of some things to do:

• We need to correct where linter is currently issuing warnings (see https://github.com/PRQL/prql/actions/runs/4302175014/jobs/7500319462, for example).
  • If we can determine that it is generating warnings that are unnecessary for us, we need to update linter's configuration files so that it no longer generates that kind of warnings.
• Turn on autofixes for formatters
• Turn on failures for the checks that we've fixed and are reliable (e.g. thanks for fixing chore: fix typo chore: fix typo #1988, we could try enforcing that if we trust it to have minimal false positives)

TODO

• php (build: update MegaLinter configs for PHP #2069)
• action (build: enable actionlint's warning from MegaLinter #2234)
• copypaste
• ropository
• yaml
• bash
• c
• cpp
• csharp
• html
• java
• json
• makefile
• markdown
• sql (build: tweak megalinter config for sql #2349)

[max-sixty]

Excellent list, thanks @eitsupi !

Unless anyone objects, I'm not sure we really need the "Needs Discussion" — we could go and do these things (but totally fine to leave it there and see if anyone does)

[nvuillam]

Indeed, you need to decide which linters are relevant for PRQL and which are not :)

• add the irrelevant ones in DISABLE_LINTERS so they don't increase running time for warnings nobody will ever look
• fix the errors detected by the linters identified as relevant ^^ (or use inline disable or repo local config files to disable issues considered not relevant: each linter documentation page has direct links about how to do that for each linter )

The second part can be done progressively of course !

[nvuillam]

About APPLY_FIXES, even if you don't activate the function to add a new commit, you can copy-paste the fixed files from the artifacts, and it will decrease the errors triggered by formatters/linters that can autofix ^^

[eitsupi]

Thank you for the detailed explanation, @nvuillam!

As @nvuillam explained, we need to decide which linter to elevate from a warning to an error, etc.
Also, I thought that perhaps it would be difficult for one person to determine that, since few people are familiar with all of the languages included in this repository.

[max-sixty]

Also, I thought that perhaps it would be difficult for one person to determine that, since few people are familiar with all of the languages included in this repository.

I'm happy to review them, it's fine to add things and then remove them later if they turn out to be a net negative. Folks should feel free to try adding them if they think it might be helpful.

My standard criteria — either it should:

• Fix all the issues it finds — e.g. a formatter, typos,
• or, Only raise errors on important things — e.g. mypy

Clippy doesn't quite fit into these, but is close enough to the second to be OK

[eitsupi]

With MegaLinter auto fix enabled, commits made with GitHub Actions do not trigger the GitHub Actions workflow under normal permissions, so it is likely that the tests required for merging will not be performed after the autofix commit is made.
If we enable auto fix, do you publish a dedicated access token for MegaLinter?

[nvuillam]

@eitsupi this is a github limitation -> https://github.com/orgs/community/discussions/27146

But it can be solvevd with a PAT, indeed :)

https://megalinter.io/latest/configuration/#apply-fixes

[eitsupi]

@nvuillam Thanks for your reply. Of course I understand that is a general limitation of GitHub Actions.

I simply do not have permission to set an access token for this repository.
(Also, managing access tokens is a pain and I prefer not to use this method. I like closing and reopening the PR to trigger workflows.)

[nvuillam]

I understand :) Or you may just add a commit locally to trigger again the workflow, instead of creating a new PR

[nvuillam]

( or just comment the part where the commit is done, and people could go in artifacts to get the updated sources and copy-paste them in their repo then push a new commit)

[max-sixty]

FYI we already the PAT for backporting, so we can experiment with this by using BACKPORT_GITHUB_TOKEN. And we could make another one / rename this one to be more general if that works.

I definitely think it's important that the tests run automatically, otherwise it's quite confusing — it's maybe OK if it's just one of us, but new contributors are not going to know to open & close the PR.

[max-sixty]

We can also move the workflow into pull-request.yaml or pull-request-target.yaml; there's no need for it to be its own workflow. I think we'll need pull-request-target.yaml if it's to add commits on

Though I do notice the workflow is extremely verbose. @nvuillam I wonder whether you might consider encapsulating some of that within an action?

[nvuillam]

@max-sixty the autofix can be performed with pre-made actions in all CI platforms, that's why we decided to not rebuild the wheel
You can remove the "create merge request" part if you don't use it :)

[max-sixty]

@max-sixty the autofix can be performed with pre-made actions in all CI platforms, that's why we decided to not rebuild the wheel

Totally agree!

I was thinking that the thing that users paste into their workflow files could instead be in MegaLinter's action.yaml, maybe accessed by some curated options. In the current state, we have this in our workflow, which isn't really viable for us to maintain, especially if there are improvements that you come up with:

      # Push new commit if applicable (for now works only on PR from same repository, not from forks)
      - name: Prepare commit
        if:
          steps.ml.outputs.has_updated_sources == 1 && (env.APPLY_FIXES_EVENT ==
          'all' || env.APPLY_FIXES_EVENT == github.event_name) &&
          env.APPLY_FIXES_MODE == 'commit' && github.ref != 'refs/heads/main' &&
          (github.event_name == 'push' ||
          github.event.pull_request.head.repo.full_name == github.repository)
        run: sudo chown -Rc $UID .git/
      - name: Commit and push applied linter fixes
        if:
          steps.ml.outputs.has_updated_sources == 1 && (env.APPLY_FIXES_EVENT ==
          'all' || env.APPLY_FIXES_EVENT == github.event_name) &&
          env.APPLY_FIXES_MODE == 'commit' && github.ref != 'refs/heads/main' &&
          (github.event_name == 'push' ||
          github.event.pull_request.head.repo.full_name == github.repository)
        uses: stefanzweifel/git-auto-commit-action@v4
        with:
          branch:
            ${{ github.event.pull_request.head.ref || github.head_ref ||
            github.ref }}
          commit_message: "[MegaLinter] Apply linters fixes"

[nvuillam]

These parts are not supposed to be updated anytime soon, they are quite standard, the updates are usually within MegaLinter core docker image :)

[eitsupi]

Given that phpstan (#2069 (comment)) and dustilock (#2319 (comment)) do not work properly with checks against only modified files, it seems that it may make sense to always inspect all code.

@max-sixty Thoughts?