Improvements to action.yml

[jsoref]

This is based on https://github.com/OpenRC/openrc/pull/523#issuecomment-1111791124

• GitHub's yaml syntax for action.yml technically requires required for each field, and the VSCode syntax validation screams about the lack of it. Note that GHA the runtime doesn't care which is why nothing explodes. And GitHub's own repositories are just as bad at not properly including these fields.
• There's no requirement to use any particular key for yaml / steps, but it's nice to be consistent. Whether you have a preference for id or name doesn't matter to me, I'm happy to use either.
• There's no particular reason to have 3 steps to do the coverity install, so I've merged it (this means I don't have to add 3 copies of the if in the next commit)
• The meat of this PR is the last commit which sets an output that is then used for each subsequent step.

[vapier]

• GitHub's yaml syntax for action.yml technically requires required for each field, and the VSCode syntax validation screams about the lack of it. Note that GHA the runtime doesn't care which is why nothing explodes. And GitHub's own repositories are just as bad at not properly including these fields.
• There's no requirement to use any particular key for yaml / steps, but it's nice to be consistent. Whether you have a preference for id or name doesn't matter to me, I'm happy to use either.

i've merged these by hand. it'd be better if detailed commit information were in the commits rather than the GH PR text.

• There's no particular reason to have 3 steps to do the coverity install, so I've merged it (this means I don't have to add 3 copies of the if in the next commit)

i avoid this in general for a few reasons:

1. make debugging output in the GHA log easier to scan & check
2. when there's a failure, it's obvious which command failed so you don't have to manually unpack the (possibly) complicated shell chain
3. working with && and || chains is error prone

i get in this particular case (3) isn't (currently) relevant, and (2) isn't that bad since there's so few commands and they're (hopefully) distinct. but i think (1) still stands.

yes, it's a bit of a bikeshedding style preference :p

[vapier]

The meat of this PR is the last commit which sets an output that is then used for each subsequent step.

yes, this is the part that really needs discussion. sorry for not getting back sooner.

first off, i completely agree that the current behavior sucks. people cloning a repo that has opted in to coverity scanning shouldn't get immediate spam/failures, especially when many people probably don't even know what coverity is or what the errors mean.

that said, i'm not seeing a way currently to balance the competing needs here. as i noted in the OpenRC PR, i really wish there was a way to configure specific actions to be disabled by default in forks such that people had to manually turn them back. but since that isn't happening anytime soon, some sort of mitigation is needed.

so what are the use cases i care about ? in order, they are:

1. projects, once the pipeline is enabled, should blow up if something goes wrong. people make mistakes all the time, and such mistakes shouldn't result in silence. this can manifest in obvious ways:

• during initial setup, a typo in the secret name (e.g. COVERITY_SCN_TOKEN) shouldn't result in passes
• if an admin accidentally deleted a secret from a repo, coverity shouldn't silently be turned off

2. when someone forks a project, their default set of actions shouldn't start failing leading to spam & confusion.
3. a forked project should be easy to keep merging with its respective upstream.
4. a forked project should be easy to opt-in to coverity, whether it be the official project, or a personal one created for the fork specifically.

the code as it stands today satisfies (1) & (3) & (4), but fails at (2).

your proposal satisfies (2), has no impact on (3) or (4), but breaks (1).

if i can only pick one, then it's def (1) over (2).

obviously forked projects could delete the action yaml file and that would resolve (2). but it'd still generate initial spam, and it wouldn't satisfy (3) or (4).

we could add another knob like enabled: ${{ secrets.COVERITY_SCAN_ENABLE }}. if the value is empty, we'd default it to "true". forked projects could set that to "false" in their secrets. it wouldn't satisfy initial spam for (2), but would let forked repos opt-out and thus satisfy (2) & (3) & (4). we'd probably also have to leave breadcrumbs in the error messages so people would be able to find them. and it'd rely on upstream projects opting in to allowing forked repos to opt out by including the enabled:... boilerplate in their action file :). maybe something like:

    - id: required-inputs
      name: Check for required inputs
      env:
        EMAIL: ${{ inputs.email }}
        TOKEN: ${{ inputs.token }}
      run:
        ...if EMAIL or TOKEN are blank, error out with explanation about missing settings, and mention that this can be disabled by setting COVERITY_SCAN_ENABLE=false...
      shell: bash

any other ideas ?

[jsoref]

Fwiw, You can use echo "::error title=something::your message".

(There's also a warning and a notice.)

Overall, I think you've done a good job at identifying the competing desires.

Fwiw, I have no objection to bike shedding style.

[jsoref]

This tries to address (1).

A real consumer would add something like:

   with:
      ...
      ignore-missing-tokens: ${{ secrets.IGNORE_MISSING_COVERITY }}

https://github.com/check-spelling/coverity-scan-action/actions/runs/2961463957

https://github.com/check-spelling/cve-bin-tool/actions/runs/2961467059

https://github.com/check-spelling/cve-bin-tool/runs/8106865992?check_suite_focus=true

Run check-spelling/coverity-scan-action@main
  with:
    build_language: other
    command: --no-command --fs-capture-search ./
    ignore-missing-tokens: 1
    project: check-spelling/cve-bin-tool
    build_platform: linux64
    working-directory: /home/runner/work/cve-bin-tool/cve-bin-tool
    version: eb65646a7567b0e6ed9bd286aa2fcde7d6d668b0
    description: coverity-scan-action check-spelling/cve-bin-tool / refs/heads/coverity

[vapier]

if i'm reading the code correctly, the new PR is basically the same as where you started. if inputs.email & inputs.token are not configured, then the build still passes, and there's no way of changing that behavior.

[jsoref]

It will either result in an error annotation or not based on the additional other parameter. If you want to have it be truly optionally fatal, I can add an extra flag for that...

[vapier]

do you have example runs without the quiet setting?

[jsoref]

These are examples where the quiet setting isn't active:

https://github.com/check-spelling/coverity-scan-action/actions/runs/2961463957

https://github.com/check-spelling/cve-bin-tool/actions/runs/2961467059

Changing the style from notice to warning or error is trivial...

[vapier]

thanks, i hadn't come across the annotations before. i couldn't find examples online of how each style renders. i suspect the only difference is the annotations are rendered with different colors?

are the annotations included in notifications sent out? I'm guessing notifications only go out when the check fails overall, and the set of annotations doesn't change that.

they only show up if you view the specific run in the actions tab right?