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

Don't abort when run on Linux / Mac OS. #172

Merged
merged 2 commits into from
Oct 27, 2021

Conversation

iamFIREcracker
Copy link
Contributor

I am trying to call this action from another composite action, one that
is supposed to be working for Linux, Mac OS, and Windows.

Ideally one would put the uses statement behind a conditional
expression, like:

- uses: msys2/setup-msys2@v2
  if: runner.os == 'Windows'

However, composite actions do not support conditional steps yet (see:
actions/runner#834), so the only way to keep
the setup-msys2 step inside my action (and not forcing the wokflow to
use it instead), is to suppress the error message (It's OK to log it,
but it should not fail).

@eine
Copy link
Collaborator

eine commented Oct 27, 2021

I understand the use case, but I don't think it's desirable to change the default behaviour. It is a legit error to use this action on an environment other than Windows, because MSYS2 is not supported anywhere else. Therefore, please, add an option to the Action for users who want the proposed behaviour to enable it explicitly. E.g. platform-check-severity. That option is pretty specific, and we can remove it in the future when composite actions support conditionals.

@eine eine added the enhancement New feature or request label Oct 27, 2021
@iamFIREcracker
Copy link
Contributor Author

I understand the use case, but I don't think it's desirable to change the default behaviour. It is a legit error to use this action on an environment other than Windows, because MSYS2 is not supported anywhere else.

I did not realize other actions might be relying on this one to error out in case the workflow was running on Linux, or Mac OS. My bad!

Therefore, please, add an option to the Action for users who want the proposed behaviour to enable it explicitly. E.g. platform-check-severity. That option is pretty specific, and we can remove it in the future when composite actions support conditionals.

Added platform-check-severity as suggested (supported values: fatal, to abort the execution, and warn to log and move on).

I even added a test, which will soon tell us if I did something wrong or not.

M.

.github/workflows/Test.yml Outdated Show resolved Hide resolved
@iamFIREcracker iamFIREcracker force-pushed the dont-fail-if-not-windows branch from 25c96ce to 428996b Compare October 27, 2021 16:42
.github/workflows/Test.yml Outdated Show resolved Hide resolved
@iamFIREcracker iamFIREcracker force-pushed the dont-fail-if-not-windows branch from 428996b to 69175fe Compare October 27, 2021 17:10
I am trying to call this action from another composite action, one that
is supposed to be working for Linux, Mac OS, and Windows.

Ideally one would put the `uses` statement behind a conditional
expression, like:

    - uses: msys2/setup-msys2@v2
      if: runner.os == 'Windows'

However, composite actions do not support conditional steps yet (see:
actions/runner#834), so the only way to keep
the setup-msys2 step inside my action (and not forcing the wokflow to
use it instead), is to suppress the error message (It's OK to log it,
but it should not fail).
@eine eine force-pushed the dont-fail-if-not-windows branch from 69175fe to f92d5e1 Compare October 27, 2021 17:16
@eine eine merged commit 12497ba into msys2:master Oct 27, 2021
@eine
Copy link
Collaborator

eine commented Oct 27, 2021

Thanks @iamFIREcracker!

@iamFIREcracker
Copy link
Contributor Author

Oh boy, in the rush I did not realize I had placed fail-fast inside matrix and not strategy...thanks for fixing this on my behalf!

@iamFIREcracker
Copy link
Contributor Author

Question: when is the next release scheduled for?

(I will be keep on using my fork for the time being, so there is no rush, really)

@eine
Copy link
Collaborator

eine commented Oct 28, 2021

Oh boy, in the rush I did not realize I had placed fail-fast inside matrix and not strategy...thanks for fixing this on my behalf!

It was my bad! I wrote the snippet incorrectly, so you were maybe mislead by that. Then, I fixed my comment, and the code 😉.

Question: when is the next release scheduled for?

ASAP. I'm waiting to see if #166 can make it into this release (depending on merging msys2/MSYS2-packages#2676).

@eine
Copy link
Collaborator

eine commented Oct 28, 2021

@iamFIREcracker, msys2/MSYS2-packages#2676 was merged already, and the package is pending signing for uploading: https://packages.msys2.org/queue. As soon as r54.fa231e8-1 is available upstream, I'll publish v2.6.0.

@eine
Copy link
Collaborator

eine commented Nov 3, 2021

@iamFIREcracker I just published v2.6.0 and updated tag v2 accordingly.

@eine eine mentioned this pull request Nov 8, 2021
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

Successfully merging this pull request may close these issues.

2 participants