search

Chocobo1 / setup-ccache-action

Setup ccache easily in your workflow, with all the tuning knobs you need!
MIT License
12 stars 4 forks source link

Consider changing Windows warning to an info printout #2

Closed AllSeeingEyeTolledEweSew closed 3 years ago

AllSeeingEyeTolledEweSew commented 3 years ago

https://github.com/Chocobo1/setup-ccache-action/blob/acabebed13c5e47f5023d0def2702873445bd0f0/src/main.ts#L120

IMO, this should be an info printout, not a warning. I'd like to be able to just use this action in a job that runs on all OSes, and have it be a no-op where not supported, rather than using conditionals to avoid the warning.

The warning creates an alert annotation in the workflow. For example: https://github.com/arvidn/libtorrent/actions/runs/1274732824

In the "PR checks" part of the actions UI, these annotations are surfaced with an exclamation mark -- See https://github.com/arvidn/libtorrent/pull/6484/checks

It appears that warnings are generally used where action/intervention is required.

Chocobo1 commented 3 years ago

IMO, this should be an info printout, not a warning. I'd like to be able to just use this action in a job that runs on all OSes, and have it be a no-op where not supported, rather than using conditionals to avoid the warning.

I'm leaning to keep it as a warning as it provides stronger indications than a normal log print: for now it really shouldn't be active on Windows platform. IMO opt-out explicitly for an action that is unsupported on some platform instead of relying on fallback behavior (no-op) is something I consider a good practice.

It appears that warnings are generally used where action/intervention is required.

Well "github actions" themselves seems to use it for mere communications: For example in your provided link (https://github.com/arvidn/libtorrent/actions/runs/1274732824) they showed a message/warning about upgrading to macOS-11 in the future. IMO this is just an info that doesn't necessarily needs user intervention.

AllSeeingEyeTolledEweSew commented 3 years ago

I thought the macOS-11 warning was a good example of an actionable alert. The workflow references macos-latest, which will change soon, so stuff could break. If the user wants to preemptively avoid breakage, that's a possible cause for action on the alert.

The windows warning isn't really actionable though. It has no effect on the build, just as a cache miss would have no effect. The only cause for action is to get rid of the warning itself.

As a user, I understand that the ccache action is a noop on windows. Lots of steps are noops in various conditions. I think having if: ${{ runner.os != "Windows" }} on a noop step in my workflow is clutter, and workflows clutter very easily, which hurts readability, so I want to aggressively reduce clutter when possible.

It sucks that the only choice is between cluttering my workflow definition, or workflow annotation output.

Chocobo1 commented 3 years ago

As a user, I understand that the ccache action is a noop on windows.

It is something I would like to reserve it as an implementation detail that nobody should be relying on: The noop is NOT a feature but a fallback for the careless user (therefore the follow-up warning).

Maybe I'll change my mind in the future but I would like to keep it as-is for now.