fix: Calculate check conclusion from annotations

[carmanchris31]

This attempts to address reviewdog/action-eslint#62 by calculating the conclusion of a check run based on the level of the annotations.

That is to say:

• if there are any error-level annotations the run is a failure
• if there are any warning- or info-level annotations the run is neutral
• otherwise, the run is a success

For reverse compatibility, the config Level takes precedence when provided.

• Updated Unreleased section in CHANGELOG or it's not notable changes.
• Added additional tests

[simllll]

Looking forward to this :-)

[simllll]

@carmanchris31 is there a reason why it's still a draft?

[carmanchris31]

@carmanchris31 is there a reason why it's still a draft?

I wanted to cover the new code with tests to make sure it works as intended. I haven’t gotten to it yet but will do so when I can (I’ll happily accept help to get this done sooner).

[Drowze]

Looking forward to this as well!

To be honest I thought -fail-on-error would already work like this... I got surprised with an error code today when trying to use it with some "INFO" level infractions 😢

[mateioprea]

If someone guides me a bit on how to approach the tests, I might be able to do them (0 experience with go). I'm also waiting for this to land so it's in our best interest. 😁

[carmanchris31]

Finally found some time to add tests for the updated conclusion method.

I just want to double check a couple things:

1. I believe the possible values for "annotation_level" are "notice", "warning", and "failure" based on https://docs.github.com/en/rest/checks/runs but can't find any guarantee that this is the case.
2. Do we want "notice"-level annotations to result in a "neutral" conclusion or "success"? I currently have it count as "neutral" but could see wanting to reserve that for warnings.

[romenrg]

Really looking forward to seeing this merged. This is a much needed feature indeed for all of us using https://github.com/reviewdog/action-eslint.

The current behavior of our GitHub Actions that use action-eslint is as follows:

• As I have tested and can be seen in the existing source code, the value of the level param seems to translate to the behavior we want as a conclusion of the action-eslint check when there are >0 annotations.
  • So, if we specify error (or don't specify a value for level, since this is the default behavior), then as soon as there is one warning (or notice), the GHA conclusion will be an failure
  • On the other hand, if we specify warning the GHA won't fail even if there are 200 errors

This behavior of the level param struck me as a weird one. But let's accept it as valid.

Having said that, what makes sense would be to have the result of the action be dependent on the type of annotations that are present in the linting output, in order to be able to actually only fail if there are errors (while being able to report warnings without failing when there are no errors).

This desired behavior is what this PR solves, and it looks good to me.
Thank you @carmanchris31 !

Looking forward to seeing this merged and being able to use a new version of action-eslint with this expected behavior soon

[romenrg]

P.S. Regarding:

Do we want "notice"-level annotations to result in a "neutral" conclusion or "success"? I currently have it count as "neutral" but could see wanting to reserve that for warnings.

I don't have a strong opinion. I think I also lean towards counting them as "neutral"

[shakefu]

@mgrachev Could you please take a look at this?

[carmanchris31]

Addressed the merge conflict

[jordemort]

Looking forward to this; after it is merged, I can fix jordemort/action-pyright#31

[adis-io]

Any update on this?

[carmanchris31]

It's been a good while with no activity. What can we do to move this forward?

[romenrg]

@haya14busa, any chance we can get this approved?
It's a pretty relevant improvement, the PR is solid and it's been several months already.
It is only pending a quick approval from a maintainer.

[danieleades]

it'd be a great christmas present. I have a couple of long-running PRs waiting on this

[danieleades]

P.S. Regarding:

Do we want "notice"-level annotations to result in a "neutral" conclusion or "success"? I currently have it count as "neutral" but could see wanting to reserve that for warnings.

I don't have a strong opinion. I think I also lean towards counting them as "neutral"

github actions don't support a neutral status do they?

[carmanchris31]

P.S. Regarding:

Do we want "notice"-level annotations to result in a "neutral" conclusion or "success"? I currently have it count as "neutral" but could see wanting to reserve that for warnings.

I don't have a strong opinion. I think I also lean towards counting them as "neutral"

github actions don't support a neutral status do they?

Probably not, but this is for the result of a check run which does.

[danieleades]

has this stalled?

[carmanchris31]

has this stalled?

@danieleades yes unfortunately... we just need the right person to approve and merge...

[jordemort]

@haya14busa Can you help with this?

[carmanchris31]

Trying again... @haya14busa can you please merge this for us?

This is a substantial quality of life improvement that has been ready for 8 months now. 🙏

[jordemort]

Perhaps @i-sevostyanov can help?

[i-sevostyanov]

LGTM

[review-dog]

Hi, @carmanchris31! We merged your PR to reviewdog! 🐶
Thank you for your contribution! ✨

We just invited you to join the @reviewdog organization on GitHub.
Accept the invite by visiting https://github.com/orgs/reviewdog/invitation.
By joining the team, you'll be a part of reviewdog community and can help the maintenance of reviewdog.

Thanks again!

[carmanchris31]

Followup required: #1426