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

Parse .tool-versions file to determine Go version #376

Closed
wants to merge 4 commits into from

Conversation

plukevdh
Copy link

@plukevdh plukevdh commented May 19, 2023

Description:
Allow setup-go to leverage the .tool-versions file for determining the Go version desired. As noted in the linked issue below

Related issue:
#375

Check list:

  • Mark if documentation changes are required.
  • Mark if tests were added or updated to cover the changes.

Please discuss:

  • This change currently makes .tool-versions the backup/fallback option if no other version spec is given. Is that desired?

  • ASDF/.tool-versions requires a fairly verboase version pattern which may or may not meet current version expectations. I see that there are a few cannonican version parsing strategies in the codebase here. Should the version be passed through those parsers or will the .tool-versions version spec be adequate?

  • I'm also unsure if the current default to .tool-versions path is going to be at project root. This is honestly my first attempt at contributing to a GH action codebase. I'd appreciate any "best practice" guidance available

  • ASDF also accepts the following version formats:

    • ref:v1.0.2-a or ref:39cb398vb39 - tag/commit/branch to download from github and compile
    • path:~/src/elixir - a path to custom compiled version of a tool to use. For use by language developers and such.
    • system - this keyword causes asdf to passthrough to the version of the tool on the system that is not managed by asdf.

    I'm not sure if any of these make sense to support given that this action currently only otherwise supports semver versions of the language.

@plukevdh plukevdh requested a review from a team as a code owner May 19, 2023 19:21
@songu21
Copy link

songu21 commented May 23, 2023

d

@songu21
Copy link

songu21 commented May 23, 2023

dddjdk

@bflad
Copy link

bflad commented May 24, 2023

Is there a particular reason why ASDF handling is being introduced into various actions rather than introducing an ASDF-specific action which parses and outputs per-tool version information? e.g. In psuedo-workflow configuration:

- uses: actions/checkout
- uses: xxx/read-asdf
  id: asdf
- uses: actions/setup-go
  with:
    go-version: steps.asdf.outputs.go

This enables downstream actions to remain generic within their domain, rather than needing to understand and maintain a different tooling ecosystem.

@bflad
Copy link

bflad commented May 24, 2023

Another potential consideration is the ASDF_DEFAULT_TOOL_VERSIONS_FILENAME environment variable:

The filename of the file storing the tool names and versions. Can be any valid filename. Typically, you should not set this value unless you want to ignore .tool-versions files.

If Unset: .tool-versions will be used.
Usage: export ASDF_DEFAULT_TOOL_VERSIONS_FILENAME=tool_versions

This is something else that could be handled once in an upstream action, rather than leaving the burden on any downstream actions to figure out the solution or potentially ignore the feature.

@plukevdh
Copy link
Author

plukevdh commented May 24, 2023

@bflad A good question. I could see a version parser being added as a separate action. My argument is that it's being added to a lot of other actions across the language ecosystem so why not add here as well. ASDF does provide an action for installing all the things, but it doesn't (and arguably shouldn't) take into account all the potential dependency caching mechanisms most of these language-specific libraries add. I could definitely look to add a tool version output parser to the ASDF action as well, but I'd still see value in this addition. I believe ASDF is attempting to put forward a tool agnostic version spec file (as opposed to a .ruby-version or .node-version file) which could be adopted more globally.

@dsame dsame added feature request New feature or request to improve the current logic investigation The issue is under investigation labels Aug 3, 2023
@dsame
Copy link
Contributor

dsame commented Aug 3, 2023

relates to #375

@dsame dsame removed the investigation The issue is under investigation label Aug 3, 2023
@dsame dsame self-assigned this Aug 4, 2023
@dsame
Copy link
Contributor

dsame commented Aug 8, 2023

@plukevdh do you think the issue can to be solved with #376 ?

@plukevdh
Copy link
Author

plukevdh commented Aug 8, 2023

@dsame can you rephrase that question? Are you asking if #402 solve this PR as well? I think it would, though I wouldn't be opposed to seeing more tests to demonstrate the solve.

@dsame dsame linked an issue Aug 15, 2023 that may be closed by this pull request
@plukevdh
Copy link
Author

Closing in favor of #402

@plukevdh plukevdh closed this Aug 16, 2023
@9777410929
Copy link

Closing in favor of #402

hi

@plukevdh plukevdh deleted the use-tool-versions branch August 19, 2023 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request to improve the current logic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Utilize the.tool-versions file to set expected Go version
5 participants