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

Fix the required field for go version #54

Merged

Conversation

naveensrinivasan
Copy link
Contributor

Signed-off-by: naveensrinivasan [email protected]

@laurentsimon
Copy link
Contributor

laurentsimon commented Apr 22, 2022

I suggest we don't force users to set this and mark it as optional as for the default actions/setup-go action.
All we need is to have two steps, where one is conditional on this value being set or not:

if: ${{ inputs.go-version != ""}}
uses: actions/setup-go@... # v2.1.3
   with:
   go-version: ${{ inputs.go-version }}

if: ${{ inputs.go-version == ""}}
uses: actions/setup-go@... # v2.1.3

(please verify this code work, I've not tested it)

What do you think?

/cc @asraa @joshuagl any opinion on this?

@laurentsimon laurentsimon self-requested a review April 22, 2022 20:50
@joshuagl
Copy link
Member

I tend to err towards setting versions explicitly (though don't want users to have to configure that in multiple places).

If we don't require an explicit go version be set, we should capture which version is used for reproducibility.

@laurentsimon
Copy link
Contributor

laurentsimon commented Apr 25, 2022

I agree that it may help us and is good practice for users to define it. Note though, that the go-version field of the underlying action can various form, e.g., >=1.17.0 - see https://github.com/actions/setup-go

Let's require it then. If we change our mind it will be easy to do so without breaking backward compatibility. The other way round won't be true.

@laurentsimon laurentsimon enabled auto-merge (squash) April 25, 2022 15:39
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.

3 participants