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

Add go-version-from-file option #62

merged 15 commits into from May 12, 2022

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

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

@bflad
Copy link

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 commented Jun 26, 2021

goodluk so I like to aplikasion

@ccremer
Copy link

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

@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 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

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

@leb4r
Copy link

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

Wow Best

@tisba
Copy link

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

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

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 commented Apr 27, 2022

@IvanZosimov
Yes! 😃
What changes are needed?

@IvanZosimov
Copy link
Contributor

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

Choose a reason for hiding this comment

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

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

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

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

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

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

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
Comment on lines 102 to 107
if (versionFilePath) {
version = installer.parseGoVersionFile(
fs.readFileSync(versionFilePath).toString(),
path.basename(versionFilePath) === 'go.mod'
);
}
Copy link
Contributor

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 00:47
@jojo43 jojo43 requested a review from a team May 1, 2022 00:47
@IvanZosimov
Copy link
Contributor

@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

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

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

@IvanZosimov Thank you for the reply. Sounds good 😸

@dmitry-shibanov dmitry-shibanov merged commit 265edc1 into actions:main May 12, 2022
@jojo43 jojo43 deleted the go-version-from-file branch May 12, 2022 23:25
panticmilos pushed a commit to panticmilos/setup-go that referenced this pull request Aug 4, 2022
n33pm pushed a commit to n33pm/setup-go that referenced this pull request Oct 17, 2022
adilhusain-s pushed a commit to adilhusain-s/setup-go that referenced this pull request Feb 6, 2023
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