-
Notifications
You must be signed in to change notification settings - Fork 527
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
Add 'go-version' Output #85
Add 'go-version' Output #85
Conversation
Just a friendly bump/ping. |
Friendly bump @brcrista @magnetikonline :) |
action.yml
Outdated
@@ -10,6 +10,9 @@ inputs: | |||
token: | |||
description: Used to pull node distributions from go-versions. Since there's a default, this is typically not supplied by the user. | |||
default: ${{ github.token }} | |||
outputs: | |||
go-version: | |||
description: 'The installed Go version. Useful when given a version range as input.' | |||
runs: | |||
using: 'node12' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcdonnnj you should do a git rebase main
- some of this is out of date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, with no attention on this pull request it fell off my radar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It happens to the best of us @mcdonnnj - just figured it might shake out any merge issues for you. 👍
src/main.ts
Outdated
// based on go/src/cmd/go/internal/version/version.go: | ||
// fmt.Printf("go version %s %s/%s\n", runtime.Version(), runtime.GOOS, runtime.GOARCH) | ||
// expecting go<version> for runtime.Version() | ||
let goVersionOutput = [...goVersion.split(' ')[2]].slice(2).join(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcdonnnj is this reliable cross-platform? Probably?
Also this is a simpler way?
let goVersionOutput = [...goVersion.split(' ')[2]].slice(2).join(''); | |
let goVersionOutput = goVersion.split(' ')[2].slice(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably avoid the use of goVersionOutput
entirely too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@magnetikonline Sorry I had to knock the cobwebs out because it's been a long while since I looked at this pull request. I'm not sure why I felt I needed to expand it to an array and join()
but yes your suggestion works fine. I'd also be happy to remove the variable entirely if that is preferred.
I just double-checked that this worked cross-platform in this Action run with jobs using ubuntu-latest
, macos-latest
, and windows-latest
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to @magnetikonline's suggestion, but we can also remove the magic number '2':
let goVersionOutput = [...goVersion.split(' ')[2]].slice(2).join(''); | |
let goVersionOutput = goVersion.split(' ')[2].slice('go'.length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be great if you could factor out this version-parsing logic to its own function and add a quick test for it. I think this is enough:
const goVersionOutput = "go version go1.16.6 darwin/amd64";
expect(parseGoVersion(goVersionOutput)).toBe("1.16.6")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@magnetikonline @brcrista Please see aa36c7f for your consideration. Please let me know if you'd like me to make any changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry was out for the weekend. @brcrista nice solution to the magic number 2
👍
This provides the semver version of Go that has been installed. This is useful if only a major or minor version has been provided as the input go-version value.
6adf460
to
2930331
Compare
@mcdonnnj looks like CI is failing. Could you run https://github.com/actions/setup-go/runs/5885728533?check_suite_focus=true#step:5:10 |
Simplify how the version is extracted and add a simple test at the same time. Co-authored-by: Peter Mescalchin <peter@magnetikonline.com> Co-authored-by: Brian Cristante <33549821+brcrista@users.noreply.github.com>
aa36c7f
to
b2f8b6e
Compare
@brcrista I've force-pushed with the corrected file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Good result all round, worthy addition 👍 |
* Add go-version to action outputs This provides the semver version of Go that has been installed. This is useful if only a major or minor version has been provided as the input go-version value. * Convert version extraction to a function Simplify how the version is extracted and add a simple test at the same time. Co-authored-by: Peter Mescalchin <peter@magnetikonline.com> Co-authored-by: Brian Cristante <33549821+brcrista@users.noreply.github.com> Co-authored-by: Peter Mescalchin <peter@magnetikonline.com> Co-authored-by: Brian Cristante <33549821+brcrista@users.noreply.github.com>
* Add go-version to action outputs This provides the semver version of Go that has been installed. This is useful if only a major or minor version has been provided as the input go-version value. * Convert version extraction to a function Simplify how the version is extracted and add a simple test at the same time. Co-authored-by: Peter Mescalchin <peter@magnetikonline.com> Co-authored-by: Brian Cristante <33549821+brcrista@users.noreply.github.com> Co-authored-by: Peter Mescalchin <peter@magnetikonline.com> Co-authored-by: Brian Cristante <33549821+brcrista@users.noreply.github.com>
* Add go-version to action outputs This provides the semver version of Go that has been installed. This is useful if only a major or minor version has been provided as the input go-version value. * Convert version extraction to a function Simplify how the version is extracted and add a simple test at the same time. Co-authored-by: Peter Mescalchin <peter@magnetikonline.com> Co-authored-by: Brian Cristante <33549821+brcrista@users.noreply.github.com> Co-authored-by: Peter Mescalchin <peter@magnetikonline.com> Co-authored-by: Brian Cristante <33549821+brcrista@users.noreply.github.com>
This provides a
go-version
output from this Action to provide the version that was installed. This is useful if you specify only a partial version such as1.15
, but need the full version installed such as1.15.5
for later use in a workflow. This is similar to the same functionality in actions/setup-python -https://github.com/actions/setup-python/blob/41b7212b1668f5de9d65e9c82aa777e6bbedb3a8/action.yml#L14-L16
Example
build.yml
: