ludeeus / action-shellcheck

GitHub action for ShellCheck.
MIT License
292 stars 69 forks source link

Not seeing annotations #55

Closed pbrisbin closed 1 year ago

pbrisbin commented 2 years ago

Describe the issue

I'm probably doing something wrong, but I can't seem to get annotations out of the problem-matcher.

Here's a run,

But no annotation for that warning is captured.

Links

(Sorry, private repository)

Config,

    steps:
      - uses: actions/checkout@v2
      - uses: ludeeus/action-shellcheck@master
        with:
          scandir: ${{ matrix.scripts }}
          severity: warning

          # This is passed to find, so using this option (istead of the intended
          # paths) will make ShellCheck run on all files in the directory
          additional_files: '-true'
ludeeus commented 2 years ago

image This is from the problem-matcher, without it it would just be a regular output like the one above and below it. Why you don't see it on the "Summary" you would have to ask GitHub about, but it's noted properly in the log itself.

pbrisbin commented 2 years ago

Hmm.

Why you don't see it on the "Summary" you would have to ask GitHub

I don't see it in the Summary or in the diff; I believe that's because the annotations are simply not being created/captured at all. Could still be GitHub's fault, but just wanted to clarify it's not a Summary-only bug.

This is from the problem-matcher, without it it would just be a regular output like the one above and below it.

FWIW, I don't believe that's necessarily true. That matches exactly the stdout of a bare shellcheck (not something parsed and re-built), except for the yellow "Warning: " prefix.

% shellcheck --format gcc ./frontend/scripts/* |& grep 'cache:11:'
./frontend/scripts/purge-imgix-cache:11:30: warning: code appears unused. Verify use (or export if used externally). [SC2034]

And I believe GitHub will add such prefixes by simply seeing that "warning:" present in the line. I've seen this happen in other Workflows, prefixing output lines when no problem-matcher is involved.

I can confirm your matcher looks good though, so it doesn't seem to be a bug in the regex:

> const re = new RegExp("^\\.?\\/?(.+):(\\d+):(\\d+):\\s(warning|error):\\s(.*)\\s\\[(SC\\d+)\\]$")
undefined
> "./frontend/scripts/purge-imgix-cache:11:30: warning: code appears unused. Verify use (or export if used externally). [SC2034]".match(re)
[
  './frontend/scripts/purge-imgix-cache:11:30: warning: code appears unused. Verify use (or export if used externally). [SC2034]',
  'frontend/scripts/purge-imgix-cache',
  '11',
  '30',
  'warning',
  'code appears unused. Verify use (or export if used externally).',
  'SC2034',
  index: 0,
  input: './frontend/scripts/purge-imgix-cache:11:30: warning: code appears unused. Verify use (or export if used externally). [SC2034]',
  groups: undefined
]

:thinking:

ludeeus commented 2 years ago

Yeah, before they did not, so I wrongly assumed they did not do it now, it seems they do. As for why/when/how it stopped I currently have no idea..

ludeeus commented 2 years ago

Actually, that highlighted warning is because of the problem-matcher. https://github.com/ludeeus/action-shellcheck/runs/5113320705?check_suite_focus=true#step:3:144 broken matcher on purpose on that run and it's no longer highlighted

pbrisbin commented 2 years ago

Ah, thanks for double-checking that.

I guess we can be sure the bug's not here?

ludeeus commented 2 years ago

Most likely not here, but I'm not sure, I have tried an older version that I know worked, and even those only show highlighting in the logs and nothing on summary or file annotations.

ludeeus commented 2 years ago

There is a problem with the filename matching at least, but the same name that is extracted by the regex works when manually echoing a ::warning:: in the action, so something has definitely changed in how the runner uses the problem matcher results. Problem matches are not really a documented feature, however using ::warning:: directly is.

So the best option is probably to remove the problem matchers, use the json as the format, iterate results and do it manually, but that would be a big breaking change.

Will try to look more on the weekend.

bdeak4 commented 2 years ago

any updates on this?