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

Migrating to json data handeling #209

Closed

Conversation

thebeline
Copy link
Contributor

@thebeline thebeline commented Apr 6, 2021

Re-Issue of #207 after fixing CheckRun resolution. All of the same comments apply:

This does a bunch... But also, not that much. In summary, it switches to using the json output of golangci-lint and handling the formatting and reporting to GitHub inside of the action. This allows for a more flexibility in how the lint issues are handled than simply passing the text output to the Action Log (which is all golangci-lint would be able to do). With this approach, the action can resolve a number of issues, and is in a better position to resolve a few more. The active changes in this PR are:

Note on Severity Levels and ignore: Failure is determined by golangci-lint exiting with a non-zero code and the Issues List containing at least one Issue with a Severity Level equal to or greater than the setting of --failure-level. This defaults to notice, and can be one of notice, warning, or failure.

Ignore is stripped from the Issues list, and therefore is neither reported to the Log, or grounds for failure (a run that reports only Issues of severity level ignore will not fail, even if golangci-lint exits with 1). It is not possible to set the failure-level to ignore either. Additionally, it is not possible to not fail on a severity level of failure.

With that in mind, this offers a few options on Conditional Failure:

  • Set the Severity of a linter to ignore, and you will not see it or fail on it.
  • Set the Severity of a linter to notice or warning, and set --failure-level to the next severity up.

Consideration: I set the default --failure-level to notice because, in the current implementation, any Lint Issue can cause a Failure, and I didn't want to change the behavior too much (that being said, handling of ignore already changes this behavior, but that seemed expected). However, it would make the most sense, semantically, to have the default --failure-level set to warning. What are your thoughts?

As you can see, if the Action has access to the full output, there is a lot that can be done fairly quickly.

With regards to Comments (#5): There is a bit of work that would need to be done to fully do this, but this sets the stage fairly nicely.

- Allows verbose reporting of Issues in Run Terminal/Log.
- Correctly maps Issue Severity to GitHub Severity
- Additionally adds support for Severity Failure levels (defaults to 'notice' [all])
- Adds suggested fixes to Annotation Raw Details
- Prepares foundation for supporting Comments
- Adds support for Multi-Line Annotations
- Adds mapping for common Issue Severities to GitHub Severities (Code climate | Checkstyle | GitHub)
- Adds handeling of 'Ignore' Issue Severity (removes from Issue List)
- Adds support for Character Range Annotations
This was surprisingly difficult.  This is pretty reliable, using low-cost methods (filtering on Name comparison, Annotation count [we add one, so it should have atleast one]) and then searching for a unique string if the count is still higher than 1.

I'm pretty happy about this, I think this will stand the test of time until GitHub get's their act together...
@thebeline thebeline marked this pull request as draft April 7, 2021 12:02
Add a Setup (Pre-Main) action to prepare the Env and begin CheckRun Resolution.

With any luck, this will solve that little issue, otherwise I give up.
I have been jumping through way too many hoops to make the non-standard checks on origin behave properly, oh well.
It is indicating it is not getting Check Suite Information, which is unlikely.
@ldez
Copy link
Member

ldez commented Jun 12, 2023

The PR handles too many topics it's impossible to review it.
Can you split your PR into several PRs? thank you.

@ldez
Copy link
Member

ldez commented Apr 29, 2024

I think it's time on close this PR.

@ldez ldez closed this Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants