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-from-file option #62

Merged
merged 15 commits into from May 12, 2022
Merged

Conversation

jojo43
Copy link
Contributor

@jojo43 jojo43 commented Jul 5, 2020

I added an option go-version-from-file as suggested in #23 (comment) .

@radeksimko
Copy link

@radeksimko radeksimko commented Jan 5, 2021

👋🏻 Is there anything in particular blocking this PR from being merged?

@bflad
Copy link

@bflad bflad commented Jun 1, 2021

Echoing the above, is there anything the community can do to help get this enhancement reviewed? It would be very useful to replace a workaround we use in a few dozen repositories that my team manages.

@driktea
Copy link

@driktea driktea commented Jun 26, 2021

goodluk so I like to aplikasion

@ccremer
Copy link

@ccremer ccremer commented Oct 19, 2021

It would be great if this option can also read and extract the go version from a go.mod file and not just take the content of the file verbatim. Otherwise, one would have to make extra steps to put the version number in a special file, which seems counter-intuitive to me.

@radeksimko
Copy link

@radeksimko radeksimko commented Oct 19, 2021

@ccremer I agree that such thing would be helpful, but it seems to me like a separate feature, as the complexity of pulling any content out of go.mod (reliably) requires parsing it.

@ccremer
Copy link

@ccremer ccremer commented Oct 19, 2021

requires parsing it.

That's what I meant with

also read and extract

I implied that extracting requires parsing in that context :)

I imagined something like

if go.mod-like-file {
  parseVersion
} else {
  useVersionVerbatim
}
catch {
  some-errorhandling
}

In any case, there are already workarounds to extract the version from go.mod.

@Weerasak2020th
Copy link

@Weerasak2020th Weerasak2020th commented Dec 20, 2021

ฉันจะเพิ่มตัวเลือกgo-version-from-fileที่แนะนำใน# 23 (ความคิดเห็น)

@leb4r
Copy link

@leb4r leb4r commented Apr 21, 2022

@ccremer I agree that such thing would be helpful, but it seems to me like a separate feature, as the complexity of pulling any content out of go.mod (reliably) requires parsing it.

I second this, some repositories do not implement a go.mod file or have no need for one. Any particular reason why this hasn't been merged yet?

@sifanjoraboris
Copy link

@sifanjoraboris sifanjoraboris commented Apr 24, 2022

Wow Best

@tisba
Copy link

@tisba tisba commented Apr 26, 2022

The Go version is already included in go.mod, so a dedicated Go version file is only needed if you really need to have an exact patch release (which sometimes can be useful of corse). In most cases I'd argue that the best practise should be to read go.mod and use the latest version.

Essentially that would be something like this: go mod edit -json | jq -r .Go.

This could be an alternative option as well. Instead "from file" a "from go mod" or something 🤔

See #174 (comment)

@radeksimko
Copy link

@radeksimko radeksimko commented Apr 26, 2022

The Go version is already included in go.mod

I think those are two different things. The version in go.mod is a compatibility constraint whereas setup-go cares about what exact version to use, which presumably should be either equal or higher than the one in go.mod. In many cases though compatibility constraint is not how you'll pick versions to use in CI AFAICT. Typically folks develop and test against particular Go versions, not just "any latest" which matches compatibility constraint.

@IvanZosimov
Copy link
Contributor

@IvanZosimov IvanZosimov commented Apr 27, 2022

Hi, @jojo43 👋 We are planning to implement this feature in the next releases of the action, but your PR is a little bit outdated and may be updated. Are you still interested in contributing 🚀 ?

@jojo43
Copy link
Contributor Author

@jojo43 jojo43 commented Apr 27, 2022

@IvanZosimov
Yes! 😃
What changes are needed?

@IvanZosimov
Copy link
Contributor

@IvanZosimov IvanZosimov commented Apr 28, 2022

Thanks for your answer, @jojo43 👍 One of our actions: setup-node, already has a pretty similar feature of getting a version of the node from a version file. You may take a look at the action's source code and use the same approach in the setup-go action.

What we are planning to implement:

  1. New input go-version-file can be added to the action. Using this input user can specify a path to the go.mod file.

  2. resolveVersionInput() We can consider that version of the go specified in the input go-version has higher priority that the version of the go which we can grab from the go.mod file. resolveVersionInput() will be able to handle the situation when user specifies both inputs. Also if only go-version-file input is specified resolver will find the desired file and transfer it to the parser function.

  3. parseGoVersionFile() - parser function, which will read the whole go.mod and look for the desired go version using regular expression.

@jojo43
Copy link
Contributor Author

@jojo43 jojo43 commented Apr 29, 2022

@IvanZosimov
Thank you for the information!
I changed the option name to go-version-file and made go.mod readable.
Correct me if I'm wrong.

Copy link
Contributor

@IvanZosimov IvanZosimov left a comment

Hi @jojo43 👋 Thank you very much for your update, it looks great, just a few changes are needed and it will brilliant ! Could I also ask you to sync your branch with the upstream branch and reattach this PR to the main branch (now it's attached to the master)?

action.yml Outdated
@@ -4,6 +4,8 @@ author: 'GitHub'
inputs:
go-version:
description: 'The Go version to download (if necessary) and use. Supports semver spec and ranges.'
go-version-from-file:
Copy link
Contributor

@IvanZosimov IvanZosimov Apr 29, 2022

Choose a reason for hiding this comment

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

Suggested change
go-version-from-file:
go-version-file:

action.yml Outdated
@@ -4,6 +4,8 @@ author: 'GitHub'
inputs:
go-version:
description: 'The Go version to download (if necessary) and use. Supports semver spec and ranges.'
go-version-from-file:
description: Path to the file with the Go version. go-version overwrites this.
Copy link
Contributor

@IvanZosimov IvanZosimov Apr 29, 2022

Choose a reason for hiding this comment

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

Suggested change
description: Path to the file with the Go version. go-version overwrites this.
description: 'Path to the go.mod file.'

Choose a reason for hiding this comment

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

@IvanZosimov FWIW as an end user I would find the original description more accurate. While I guess none of us can predict what % of users will end up specifying go.mod and what other % .go-version, but I assume (hope) that the intention is to support both and so the description should reflect that?

Copy link
Contributor

@IvanZosimov IvanZosimov May 4, 2022

Choose a reason for hiding this comment

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

Hi, @radeksimko 👋 ! Thank you, that's definitely a good point! We are going to update setup-go documentation with the new chapter related to this functionality with a more broad description of the input.

function resolveVersionInput(): string {
let version = core.getInput('go-version');
const versionFilePath = core.getInput('go-version-file');

Copy link
Contributor

@IvanZosimov IvanZosimov Apr 29, 2022

Choose a reason for hiding this comment

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

I would add here the warning when the user specifies both inputs:

if (version && versionFilePath) {
    core.warning(
      'Both go-version and go-version-file inputs are specified, only go-version will be used'
    );
  }

src/installer.ts Outdated
@@ -266,3 +266,12 @@ export function makeSemver(version: string): string {

return `${verPart}${prereleasePart}`;
}

export function parseGoVersionFile(contents: string, isMod: boolean): string {
Copy link
Contributor

@IvanZosimov IvanZosimov Apr 29, 2022

Choose a reason for hiding this comment

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

As the input go-version-file should contain the path to a go.mod file (we'll deliberately ask our customers to put there a path to go.mod file ), I'd suggest not using isMod parameter in this function and don't check whether path refers to 'go.mod' file.

src/main.ts Outdated
if (versionFilePath) {
version = installer.parseGoVersionFile(
fs.readFileSync(versionFilePath).toString(),
path.basename(versionFilePath) === 'go.mod'
);
}
Copy link
Contributor

@IvanZosimov IvanZosimov Apr 29, 2022

Choose a reason for hiding this comment

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

I'd suggest keeping this conditional statement very similar to the one in setup-node action. Otherwise, some important functionality will be lost.

@jojo43 jojo43 changed the base branch from master to main May 1, 2022
@jojo43 jojo43 requested a review from as a code owner May 1, 2022
@jojo43 jojo43 requested a review from IvanZosimov May 1, 2022
@jojo43
Copy link
Contributor Author

@jojo43 jojo43 commented May 1, 2022

@IvanZosimov
Please take a look again.

@dmitry-shibanov
Copy link
Contributor

@dmitry-shibanov dmitry-shibanov commented May 1, 2022

Hello @jojo43. Could you please add new job in versions.yml with go-version-file input.

@panchoh
Copy link

@panchoh panchoh commented May 2, 2022

The Go version is already included in go.mod

I think those are two different things. The version in go.mod is a compatibility constraint whereas setup-go cares about what exact version to use, which presumably should be either equal or higher than the one in go.mod. In many cases though compatibility constraint is not how you'll pick versions to use in CI AFAICT. Typically folks develop and test against particular Go versions, not just "any latest" which matches compatibility constraint.

Thanks a bunch to @jojo43 for pushing this!

Nevertheless, please take into consideration @radeksimko remarks, because they are spot-on, IMHO. For a CI-CD workflow, you typically want to select the exact point revision you are using, not what happens to be the last one available (if you want reproducible builds, that is). Otherwise you can end up with a non-functioning build, and be flabbergasted about why. For instance, in the 1.16.x or 1.17.x series there was a point release that changed the behavior of the web server, requiring changes to code using it, otherwise your program wouldn't build.

I wholeheartedly recommend the approach shown here, where the Go version is read from a Dockerfile. It's genius! Because then, you can trivially configure dependabot to look for updates to the Dockerfile, and since the Go project publishes updated Docker images as part of each release, you can keep your build updated with very little effort, and in a controlled fashion. Dependabot will produce a PR with the updated Dockerfile pointing to the latest Go release available. You can accept (or not) the PR, and get yourself an updated build. If you have tests, the PR will run those, and you get feedback on the effects of the change. Works like a charm!

My 2¢.

Thanks again!

@IvanZosimov
Copy link
Contributor

@IvanZosimov IvanZosimov commented May 4, 2022

@IvanZosimov Please take a look again.

Hi, @jojo43 👋 Thanks a lot for your updates! Could you also update documentation (the README.md file) and add there this chapter:

## Getting go version from the go.mod file

The `go-version-file` input accepts a path to a `go.mod` file containing the version of Go to be used by a project. As the `go.mod` file contains only major and minor (e.g. 1.18) tags, the action will search for the latest available patch version sequentially in the runner's directory with the cached tools, in the [version-manifest.json](https://github.com/actions/go-versions/blob/main/versions-manifest.json) file or at the go servers. 

If both the `go-version` and the `go-version-file` inputs are provided then the `go-version` input is used.
> The action will search for the `go.mod` file relative to the repository root

```yaml
steps:
- uses: actions/checkout@v3
- uses: actions/setup-go@v3
  with:
    go-version-file: '**/go.mod'
- run: go version

Shifted chapter related to retrieving go version from go.mod file and
updated yml example
@minamijoyo
Copy link

@minamijoyo minamijoyo commented May 11, 2022

Hi all, Thank you for working on this!

While reading this thread, I'm not sure why the .go-version file for goenv won't be supported as mentioned by @radeksimko, @panchoh and also described in the original description of this pull request.

Is it just an out of scope for the initial implementation? or it won't be supported by design?

@IvanZosimov
Copy link
Contributor

@IvanZosimov IvanZosimov commented May 11, 2022

Hi, @minamijoyo 👋 Thanks a lot for your concerns, actually you are right there, the scope of this PR is to add retrieving Go version from go.mod file functionality. Nevertheless, you can specify path to .go-version file in the input go-version-file and our action will read the version from it. We will update the documentation with a note about that later after merging this PR.

Cheers!

@minamijoyo
Copy link

@minamijoyo minamijoyo commented May 11, 2022

@IvanZosimov Thank you for the reply. Sounds good 😸

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