Add MegaLinter #679

vkucera · 2022-04-14T10:18:59Z

This adds MegaLinter as a CI action to lint and format code other than C++ and CMake.

Fixes and formatting have been applied to the existing files to pass the tests, with the exception of Scripts/update_ccdb.py which requires some more than trivial fixes which I leave to the next editor.

The action is set to check only modified files with VALIDATE_ALL_CODEBASE: false.

If a PR passes the tests without errors but formatting is still required, a formatting commit is committed directly to the PR branch. This is set with APPLY_FIXES_MODE: commit. (The option to create a PR instead has some problems with GitHub tokens AFAIK.)

davidrohr · 2022-04-14T11:17:54Z

I don't really like committing to peoples branches directly. I'd investigate the option to create a PR instead, and otherwise just drop it.

vkucera · 2022-04-14T11:53:20Z

I don't really like committing to peoples branches directly. I'd investigate the option to create a PR instead, and otherwise just drop it.

Hi @davidrohr , the advantage is that the PR author does not need to care about reading instructions on how to run formatting tools or to approve a PR in their fork repo. What disadvantage do you see there?

davidrohr · 2022-04-14T11:54:58Z

I don't like to concept of pushing in other people's repository. Then I'll just mark my PRs as not editable by maintainers...

davidrohr · 2022-04-14T11:56:51Z

Also, this adds a commit on top, which should normally be squashed, but the automatic commit cannot squash properly if the PR consists of multiple commits, and also this would require a force push to the user's repository, which I also think we should not do.

vkucera · 2022-04-14T16:23:42Z

Also, this adds a commit on top, which should normally be squashed, but the automatic commit cannot squash properly if the PR consists of multiple commits, and also this would require a force push to the user's repository, which I also think we should not do.

Squashing the automatic formatting commit with other commits in the PR works fine.

jgrosseo · 2022-04-14T17:41:09Z

The clang formatter makes a PR which the user can import or not. I think this would be a good solution also here. What's speak against this solution?

vkucera · 2022-04-19T13:28:16Z

The clang formatter makes a PR which the user can import or not. I think this would be a good solution also here. What's speak against this solution?

Does it? I cannot find a single PR created by the formatting action.

TimoWilken · 2022-04-19T13:32:54Z

Does it? I cannot find a single PR created by the formatting action.

These PRs are created in the PR author's fork. For example: vkucera/AliceO2#10. That is done by this code.

vkucera · 2022-04-19T13:55:00Z

Does it? I cannot find a single PR created by the formatting action.

These PRs are created in the PR author's fork. For example: vkucera/AliceO2#10. That is done by this code.

Thanks @TimoWilken , I am aware of this feature. My point is that I cannot find an example of it working in O2Physics.
If I look through the runs of the action where it failed, I don't see any that would create a PR in the author's branch.
For example: #650 clearly needed a formatting commit which was made and pushed by the author. The corresponding run of clang-format does not seem to have made any PR in the concerned branch though.
I only see errors of this kind:

 ! [remote rejected] HEAD -> alibot-cleanup-670 (refusing to allow a Personal Access Token to create or update workflow `.github/workflows/code-formatting.yml` without `workflow` scope)
error: failed to push some refs to 'https://github.com/alibuild/O2Physics.git'

TimoWilken · 2022-04-19T14:03:56Z

Ah, right. The action had the wrong permissions -- that should be fixed now.

vkucera · 2022-04-19T14:17:35Z

Ah, right. The action had the wrong permissions -- that should be fixed now.

Thanks! I will try to put that in this one.

github-actions · 2022-05-22T01:47:32Z

This PR has not been updated in the last 30 days. Is it still needed? Unless further action is taken, it will be closed in 5 days.

TimoWilken

Looks pretty good overall, though I have a few questions -- see below.

.github/workflows/mega-linter.yml

TimoWilken · 2022-05-23T13:38:22Z

.github/workflows/mega-linter.yml

+      - name: Exit with error if the PR is not clean
+        run: |
+          case "${{ steps.ml.outputs.has_updated_sources }}" in
+            0) echo 'PR clean' ; exit 0 ;;
+            1) echo 'PR not clean' ; exit 1 ;;
+          esac


How is the case handled where MegaLinter finds problems, but does not have automatic fixes for them? In that case, we still want to exit with an error, right?

Yes, in case of errors, the MegaLinter step fails.

In that case, the automatic PR with any fixes that MegaLinter does produce wouldn't be created, right?

That is correct. The formatting PR should be created only if MegaLinter did not find any errors, which I think is the intended behaviour, right?

.github/workflows/mega-linter.yml

TimoWilken

Looks good to me. If the current codebase (with changes in this PR) passes MegaLinter, then let's merge and try it out!

vkucera · 2022-06-02T11:43:22Z

Looks good to me. If the current codebase (with changes in this PR) passes MegaLinter, then let's merge and try it out!

Thanks @TimoWilken

jgrosseo · 2022-06-02T14:16:05Z

Looks good to me. If the current codebase (with changes in this PR) passes MegaLinter, then let's merge and try it out!

How do we check this? Let me know, I can then merge it.

vkucera · 2022-06-02T14:22:44Z

@TimoWilken Is the action supposed to run already on this PR itself?
The original version which was set to run on push and pull_request did run but now it is set to run on pull_request_target only.

vkucera · 2022-06-02T14:39:21Z

@TimoWilken Is the action supposed to run already on this PR itself? The original version which was set to run on push and pull_request did run but now it is set to run on pull_request_target only.

Otherwise, the current code passes all the tests except for a few bandit issues which should be addressed at the next modification of Scripts/update_ccdb.py .

TimoWilken · 2022-06-07T12:28:17Z

Seeing as the new linter checks passed before, let's merge it and see what happens.

TimoWilken · 2022-06-07T19:04:55Z

FYI: I had to add one more thing (92b5340) to make the automatic cleanup PRs work. Apart from that, looks like it runs OK.

vkucera · 2022-06-07T22:13:03Z

FYI: I had to add one more thing (92b5340) to make the automatic cleanup PRs work. Apart from that, looks like it runs OK.

Thanks @TimoWilken . I'm glad it seems to work now.

* Add MegaLinter * Fix YAML * Fix formatting * Add Flake8 config file * Check only modified files * Allow formatting commits on push events * Integrate alisw/pull-request * Comments by Timo

vkucera marked this pull request as ready for review April 14, 2022 11:08

vkucera requested review from a team, alibuild, iarsene, jgrosseo, ktf and pbuehler as code owners April 14, 2022 11:08

vkucera requested a review from TimoWilken April 14, 2022 11:08

vkucera force-pushed the megalinter branch 2 times, most recently from 7d36dcf to 6246108 Compare April 20, 2022 15:36

vkucera marked this pull request as draft April 21, 2022 09:29

vkucera force-pushed the megalinter branch from 6246108 to 33a2596 Compare April 21, 2022 10:24

github-actions bot added the stale label May 22, 2022

vkucera force-pushed the megalinter branch 2 times, most recently from 796e84d to b5ceac8 Compare May 22, 2022 22:41

TimoWilken requested changes May 23, 2022

View reviewed changes

github-actions bot closed this May 30, 2022

vkucera reopened this May 30, 2022

vkucera added 8 commits May 30, 2022 17:28

Add MegaLinter

496663a

Fix YAML

90de4bd

Fix formatting

0563476

Add Flake8 config file

beb5720

Check only modified files

5b89f26

Allow formatting commits on push events

40bfe06

Integrate alisw/pull-request

a37abee

Comments by Timo

9c04270

vkucera force-pushed the megalinter branch from 673d8f1 to 9c04270 Compare May 30, 2022 15:30

vkucera requested a review from TimoWilken May 30, 2022 16:43

TimoWilken approved these changes Jun 2, 2022

View reviewed changes

vkucera marked this pull request as ready for review June 2, 2022 11:42

TimoWilken merged commit 5f3eaca into AliceO2Group:master Jun 7, 2022

vkucera deleted the megalinter branch June 7, 2022 12:30

Add MegaLinter #679

Add MegaLinter #679

Uh oh!

Conversation

vkucera commented Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidrohr commented Apr 14, 2022

Uh oh!

vkucera commented Apr 14, 2022

Uh oh!

davidrohr commented Apr 14, 2022

Uh oh!

davidrohr commented Apr 14, 2022

Uh oh!

vkucera commented Apr 14, 2022

Uh oh!

jgrosseo commented Apr 14, 2022

Uh oh!

vkucera commented Apr 19, 2022

Uh oh!

TimoWilken commented Apr 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vkucera commented Apr 19, 2022

Uh oh!

TimoWilken commented Apr 19, 2022

Uh oh!

vkucera commented Apr 19, 2022

Uh oh!

github-actions bot commented May 22, 2022

Uh oh!

TimoWilken left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TimoWilken May 23, 2022

Choose a reason for hiding this comment

Uh oh!

vkucera May 23, 2022

Choose a reason for hiding this comment

Uh oh!

TimoWilken May 23, 2022

Choose a reason for hiding this comment

Uh oh!

vkucera May 24, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TimoWilken left a comment

Choose a reason for hiding this comment

Uh oh!

vkucera commented Jun 2, 2022

Uh oh!

jgrosseo commented Jun 2, 2022

Uh oh!

vkucera commented Jun 2, 2022

Uh oh!

vkucera commented Jun 2, 2022

Uh oh!

TimoWilken commented Jun 7, 2022

Uh oh!

TimoWilken commented Jun 7, 2022

Uh oh!

vkucera commented Jun 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

vkucera commented Apr 14, 2022 •

edited

Loading

TimoWilken commented Apr 19, 2022 •

edited

Loading