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

fs.Version to support version constrained binary discovery #52

Merged
merged 9 commits into from May 10, 2022

Conversation

magodo
Copy link
Contributor

@magodo magodo commented Apr 11, 2022

Allows users to optionally specify one or more (comma separated) version constraints for fs.AnyVersion source.

Allows users to optionally specify one or more (comma separated) version constraints for `fs.AnyVersion` source.
@radeksimko
Copy link
Member

Hi @magodo
Just for consistency with other parts of our API, in particular the sources under releases package - e.g. https://pkg.go.dev/github.com/hashicorp/hc-install@v0.3.1/releases#LatestVersion would you mind exposing this as a new fs.Version source instead of modifying fs.AnyVersion?

Aside from the name communicating better what the source represents we also wouldn't need to deal with conflicts with ExactBinPath.

@magodo
Copy link
Contributor Author

magodo commented Apr 12, 2022

@radeksimko That is what I did at the first try :) But I didn't come up with a good name for that. Do you have any suggestion about the naming?

@magodo
Copy link
Contributor Author

magodo commented May 4, 2022

@radeksimko A kindly ping, would you mind provide a suitable name for the new source?

@radeksimko
Copy link
Member

@magodo I already proposed the new name in my original comment 😉

would you mind exposing this as a new fs.Version source instead of modifying fs.AnyVersion?

@magodo
Copy link
Contributor Author

magodo commented May 4, 2022

@radeksimko Ah, sorry 🙈. I've made a new commit to implement the fs.Version source, please let me know if anything else is needed.

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Thanks!

I left you an in-line comment regarding the tests - otherwise the implementation LGTM.

fs/fs_test.go Outdated Show resolved Hide resolved
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

This is looking pretty good, I just left one suggestion, effectively re-enabling the test. Aside from that I think you may need to rebase your branch, to bring in #57

Then once tests are passing I'm happy to approve and merge.

fs/fs_test.go Outdated Show resolved Hide resolved
@magodo
Copy link
Contributor Author

magodo commented May 10, 2022

@radeksimko I've updated the test and merged with main branch. The test is passing now:

❯ E2E_TESTING=1 go test -v -run="^TestVersion$" ./...
=== RUN   TestVersion
--- PASS: TestVersion (7.79s)
PASS
ok      github.com/hashicorp/hc-install/fs      7.795s

@radeksimko radeksimko changed the title fs.AnyVersion supports optional version constraint fs.Version to support version constraint May 10, 2022
@radeksimko radeksimko changed the title fs.Version to support version constraint fs.Version to support version constrained binary discovery May 10, 2022
@radeksimko radeksimko merged commit d615262 into hashicorp:main May 10, 2022
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

2 participants