Skip to content

Conversation

@tbroyer
Copy link
Contributor

@tbroyer tbroyer commented May 25, 2020

This allows calling the action multiple times in the
same job and retrieving the path and/or version in
other steps.

Fixes #65

if (!$args.Count -or !$args[0])
{
throw "Must supply java version argument"
throw "::error::Must supply java version argument"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for submitting this PR.

The only question I have is regarding this ::error:: syntax. Is there any benefit to doing this? I haven't seen any of our other first party actions use this (I want to maintain some level of consistency). Online I can't seen to find anything either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is https://help.github.com/en/actions/reference/workflow-commands-for-github-actions#setting-an-error-message, as seen in @actions/core's error() too as an implementation detail.

Not sure about the throw in PowerShell though, but according to https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_throw?view=powershell-7 this should work too.

Copy link
Contributor

@konradpabjan konradpabjan May 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played around this, it makes the error message show up red in the logs so it's a pretty nice addition 😃

image

It doesn't fail the step though so exit 1 is still needed (for bash).

If Powershell it's redundant as throw will make the logs show up red and it will fail the step so the extra ::errror:: is not needed

image

This allows calling the action multiple times in the
same job and retrieving the path and/or version in
other steps.

Fixes actions#65
@tbroyer
Copy link
Contributor Author

tbroyer commented May 27, 2020

Force-pushed with the PowerShell changes.

@konradpabjan
Copy link
Contributor

Did some extra testing and everything is 👍

Thanks for the contribution @tbroyer 🎉🎈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add output parameter with path to installed JDK

2 participants