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

Change report when FAIL_IF_UPDATED_SOURCES is true. #1493

Closed
melg8 opened this issue May 26, 2022 · 2 comments
Closed

Change report when FAIL_IF_UPDATED_SOURCES is true. #1493

melg8 opened this issue May 26, 2022 · 2 comments
Labels
enhancement New feature or request O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity

Comments

@melg8
Copy link

melg8 commented May 26, 2022

Current behavior

When you set FAIL_IF_UPDATED_SOURCES to true, github pull comment is visualized as SUCCESS overall. And only log contains message "Sources has been updated by linter auto-fixes, and FAIL_IF_UPDATED_SOURCES has been set to true".
For new user of project which uses MegaLinter this behavior is not very intuitive. Values in "Fixed" column could be not noticed at all or misinterpreted, because of SUCCESS status and ✅ symbol on the linter line. So user will need to do additional actions to figure out why overall build failed, when GitHub report says that all passed:

Current failure investigation path looks like that:

  1. Pull request failure
  2. Going inside - Megalinter comment status SUCCESS (confusion)
  3. Scrolling down to github status - all linters successfull (confusion x2)
  4. Megalinter task itself failed (confusion x3)
  5. Click details, scroll to bottom
  6. Viewing message "Sources has been updated by linter auto-fixes, and FAIL_IF_UPDATED_SOURCES has been set to true"
  7. Confusion one more time because of message. Does that mean, that problem solved itself? or does user need to do something additionally to fix it?
  8. From which linter all this came from? etc.
  9. What changed/suggested? going back to pull request.

After suggested changes it should look something like that:

  1. Pull request failure
  2. Going inside - Megalinter comments status ERROR (okay, got it)
  3. Concrete linter - Error sign (okay, reduced scope of search)
    4.1 Message at the end of comment: You can fix them by applying fixes which will trigger new MegaLinter check (okay cool got it). - this is for projects, which are just suggesting changes, but don't act on behalf of the user.
    Or
    4.2 We applied all fixes for you, just wait a bit and new status will appear (great!).

Suggested behavior

When FAIL_IF_UPDATED_SOURCES set to true. GitHub report should display:

  • "MegaLinter status: ERROR" as title.
  • github pull comment display ERROR status on line of linter which suggested new changes.
  • number in Fixed column could have link to artifacts, same as displayed for elements in Errors column.
  • github status reporter should contain error sign for linter which suggested new changes.
  • maybe additional comment at the end of github pull comment to notify user about why that happened "FAIL_IF_UPDATED_SOURCES has been set to true". And how he can apply suggested fixes.
  • maybe redact original "Sources has been updated by linter auto-fixes, and FAIL_IF_UPDATED_SOURCES has been set to true" - to something mentioning what user can do to fix this problem. (apply suggested changes and rerun MegaLinter).
  • same behavior for other reporters (like gitlab one) if it has same issue (didn't check it myself).
  • maybe same behavior of report for local run too.

To @nvuillam and MegaLinter contributors - this kind of issues are more like life quality ideas rather than critical issues. But i thought MegaLinter is all about improving linting experience for users and providing useful information as fast as possible to the end users. After all, running linters by hand or with bash scripts can do the job too. I have some more ideas in direction of improving UI of the tool, but I'm not very experienced with python to implement them myself. If I'm setting too high bar for MegaLinter usability or such changes are not in current priority, tell me, and I will not bother you with such kind of issues.

@melg8 melg8 added the enhancement New feature or request label May 26, 2022
@Kurt-von-Laven
Copy link
Collaborator

Kurt-von-Laven commented May 26, 2022

Thank you for filing this issue so thoughtfully. I agree with your proposal. For context, the FAIL_IF_UPDATED_SOURCES config option was added very recently and in a minimal way in response to user feedback since we try to take an iterative approach. This has the drawback you are right to point out that there are many improvements that can be made to the behavior of this config setting. My suspicion is that the easiest way to approach this issue would be to find ways to keep all of the reporters as consistent with each other as possible, although I am not yet familiar with that portion of the code base. We are happy to accept pull requests that progress towards the direction you have so thoroughly laid out here.

In the meantime, in case it appeals to your team or others, I will describe how we are using MegaLinter a little differently than most with the caveat that it's not a usability panacea. My team runs MegaLinter as a pre-commit hook both locally and in CI. It is easy to run MegaLinter as a pre-commit hook if this appeals to you. The megalinter hook runs incrementally pre-commit by default, so we have been satisfied with its performance. The megalinter-all hook runs on all files pre-push by default for thoroughness, but nonetheless runs much faster on our laptops than it would in CI since the GitHub-hosted runners aren't very powerful. The pre-commit framework treats all hooks that modify sources as failing, and expects hooks to log minimal output to the console since many hooks may be run. Hence, we are neither relying on FAIL_IF_UPDATED_SOURCES nor the GitHub Action nor any of the reporters on a regular basis. We are instead looking in report/linters_logs/ for more detailed error messages and the output of git diff for auto-fixes.

@github-actions
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 Jun 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity
Projects
None yet
Development

No branches or pull requests

2 participants