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

only_warn should produce warnings not errors #78

Open
doc-sheet opened this issue Aug 22, 2024 · 11 comments
Open

only_warn should produce warnings not errors #78

doc-sheet opened this issue Aug 22, 2024 · 11 comments
Labels
enhancement New feature or request

Comments

@doc-sheet
Copy link

doc-sheet commented Aug 22, 2024

Now all messages show as ::error regardless of only_warn value.

@DimitriPapadopoulos
Copy link
Collaborator

DimitriPapadopoulos commented Aug 22, 2024

What do you mean by "Now"? Did anything change recently? Since when?

How do you set only_warn exactly?

@doc-sheet
Copy link
Author

now == last time i've tested it - few days ago at latest version I believe

It works well as action

      - name: check spelling
        uses: codespell-project/actions-codespell@406322ec52dd7b488e48c1c4b82e2a8b3a1bf630
        with:
          only_warn: true
          check_filenames: true
          path: >-
				some paths here

The only minor issue - one i'm talking about - is only_warn makes codespell to not fail action run by using return code 0.
But messages it pins on PR diff still show as errors.

@DimitriPapadopoulos
Copy link
Collaborator

I see. So it's not a new or recent issue. The word "New" simply means you're running the latest version.

@DimitriPapadopoulos
Copy link
Collaborator

I have no clue how to fix this myself. If you know, please do not hesitate to provide a pull request.

@DimitriPapadopoulos
Copy link
Collaborator

But then, it looks like only_warn does what it is supposed to do:

Only warn about problems. All errors and warnings are annotated in Pull Requests, but it will act like everything was fine anyway. (In other words, the exit code is always 0.)

@doc-sheet
Copy link
Author

Yeah i just suggest to replace all error annotations as warnings. Sadly I don't fully understand how this action works so I can't suggest proper fix.

Looks like

should be WARNING always if only_warn is enabled.

@DimitriPapadopoulos
Copy link
Collaborator

But that's not the documented behaviour as far as I an tell. That would be a breaking change involving a major version bump, wouldn't it?

On the other hand, that's what other actions do. See for example https://github.com/actions/dependency-review-action:

warn-only

When set to true, the action will log all vulnerabilities as warnings regardless of the severity, and the action will complete with a success status. This overrides the fail-on-severity option.

Finally, what exact difference does that make in practice? How do warnings and errors differ in the output of CI jobs?

@DimitriPapadopoulos DimitriPapadopoulos added enhancement New feature or request documentation Improvements or additions to documentation and removed documentation Improvements or additions to documentation labels Aug 24, 2024
@doc-sheet
Copy link
Author

what exact difference does that make in practice?

It is just a small recolor of things :)
warning
error

@doc-sheet
Copy link
Author

not the documented behaviour as far as I an tell.

TBH I see that matcher json for first time and I didn't even see docs of how to use it.

Usually i just write something like echo ::error::blabla or echo ::warning::blabla where it is easy to control

@peternewman
Copy link
Collaborator

So I based this action on this one and just took the names from there:
https://github.com/TrueBrain/actions-flake8?tab=readme-ov-file#parameter-only_warn

FWIW the matcher stuff is documented here:
https://github.com/actions/toolkit/blob/main/docs/problem-matchers.md

The main benefit is you don't need to make any changes to the tool generally, you just write some Regex to match its output (which also makes things like https://github.com/codespell-project/sort-problem-matcher possible where they probably wouldn't accept a PR to modify sort for GitHub usage).

Unfortunately if the docs are correct you can't just overwrite the number in that config as they Regex group matches, you'd have to either modify Codespell itself or pre-process its output.

Personally I'd say there's an argument for both use cases, you might want to flag the correct severity of found issues, whilst not failing the whole build (as it currently does) but maybe in retrospect that should have been a differently named option. Then there's what you're asking for where all logged alerts are at the warning level. But do you also want the job to be forced success too?

@peternewman
Copy link
Collaborator

Correction, apparently their docs are rubbish and you can sort of override severity, but you'd need a different matcher:
https://github.com/orgs/community/discussions/5924#discussioncomment-5722148

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants