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

Add 'go-version' Output #85

Merged
merged 2 commits into from Apr 8, 2022

Conversation

mcdonnnj
Copy link
Contributor

@mcdonnnj mcdonnnj commented Nov 18, 2020

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 as 1.15, but need the full version installed such as 1.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:

jobs:
  test:
    runs-on: ubuntu-latest
    steps:
      - id: setup-go
        uses: action/setup-go@v2.1.4
        with:
          go-version: '1.15'
      - name: Lookup go cache directory
        run: echo "GOCACHE_DIR=$(go env GOCACHE)" >> $GITHUB_ENV
      - uses: actions/cache@v2
        with:
          path: |
            ${{ env.GOCACHE_DIR }}
          key: "test-${{ runner.os }}-go${{ steps.setup-go.outputs.go-version }}"

@mcdonnnj
Copy link
Contributor Author

@mcdonnnj mcdonnnj commented Feb 7, 2021

Just a friendly bump/ping.

@mvdan
Copy link

@mvdan mvdan commented Apr 7, 2022

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'
Copy link
Contributor

@magnetikonline magnetikonline Apr 7, 2022

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.

Copy link
Contributor Author

@mcdonnnj mcdonnnj Apr 7, 2022

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.

Copy link
Contributor

@magnetikonline magnetikonline Apr 7, 2022

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('');
Copy link
Contributor

@magnetikonline magnetikonline Apr 7, 2022

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?

Suggested change
let goVersionOutput = [...goVersion.split(' ')[2]].slice(2).join('');
let goVersionOutput = goVersion.split(' ')[2].slice(2);

Copy link
Contributor

@magnetikonline magnetikonline Apr 7, 2022

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?

Copy link
Contributor Author

@mcdonnnj mcdonnnj Apr 7, 2022

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.

Copy link
Contributor

@magnetikonline magnetikonline Apr 7, 2022

Choose a reason for hiding this comment

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

Lovely 👍

Copy link
Contributor

@brcrista brcrista Apr 7, 2022

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':

Suggested change
let goVersionOutput = [...goVersion.split(' ')[2]].slice(2).join('');
let goVersionOutput = goVersion.split(' ')[2].slice('go'.length);

Copy link
Contributor

@brcrista brcrista Apr 7, 2022

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")

Copy link
Contributor Author

@mcdonnnj mcdonnnj Apr 8, 2022

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.

Copy link
Contributor

@magnetikonline magnetikonline Apr 10, 2022

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 👍

@magnetikonline
Copy link
Contributor

@magnetikonline magnetikonline commented Apr 7, 2022

@mvdan I left some comments - but I don't have merge rights (I'm not a GitHub staffer!).

Over to @brcrista 👍 😄

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.
@mcdonnnj mcdonnnj force-pushed the improvement/add_go-version_output branch from 6adf460 to 2930331 Compare Apr 7, 2022
@mcdonnnj mcdonnnj requested a review from as a code owner Apr 7, 2022
@brcrista
Copy link
Contributor

@brcrista brcrista commented Apr 8, 2022

@mcdonnnj looks like CI is failing. Could you run npm run format-check and fix any issues?

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>
@mcdonnnj mcdonnnj force-pushed the improvement/add_go-version_output branch from aa36c7f to b2f8b6e Compare Apr 8, 2022
@mcdonnnj
Copy link
Contributor Author

@mcdonnnj mcdonnnj commented Apr 8, 2022

@mcdonnnj looks like CI is failing. Could you run npm run format-check and fix any issues?

https://github.com/actions/setup-go/runs/5885728533?check_suite_focus=true#step:5:10

@brcrista I've force-pushed with the corrected file.

Copy link
Contributor

@brcrista brcrista left a comment

Thanks!

@brcrista brcrista merged commit 4a4352b into actions:main Apr 8, 2022
34 checks passed
@mcdonnnj mcdonnnj deleted the improvement/add_go-version_output branch Apr 8, 2022
@magnetikonline
Copy link
Contributor

@magnetikonline magnetikonline commented Apr 10, 2022

Good result all round, worthy addition 👍

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.

None yet

4 participants