-
Notifications
You must be signed in to change notification settings - Fork 22
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
command line interface #60
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library started out with a relatively flexible (Go) API and a minimal dependency tree, both of which is also what motivated https://github.com/hashicorp/hc-install#hc-install-is-not-a-package-manager and make it possible for e.g. Terraform SDK to use this without much cost. It would be great if we could keep it that way. I'm happy with With all that in mind:
I understand I'm suggesting some significant changes to your PR, but I hope the context and explanation above can help understand why. Either way - thank you for kicking this off and raising the PR and thank you for the patience. Let me know what you think! |
This all sounds reasonable @radeksimko - but I am wondering about the default value for
What is this referring to? I was basing the idea off of |
Thanks for this! Agree with @radeksimko's comments above. In particular regarding the default installation path, note that as per the README,
I think therefore the default path for downloaded binaries should be the current directory, the analogy being While |
@kmoe I like that idea 👍🏻 and I like your analogy with @tgross How does that sound as a default option for
I was just referring to the Go API of this library rather than hc-install/releases/latest_version.go Line 82 in af09eee
|
Works for me. I'll take a pass at updating this PR with your suggestions sometime in the next couple days probably. |
9a5f13b
to
8fec2ca
Compare
I've updated this PR as follows:
@radeksimko I thought I understood this comment:
But then when I went to look at it I don't think I understand what you're asking for here. 😊 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @tgross
This is getting quite close to a merge-able state. I left some suggestions in-line.
@radeksimko I've updated the PR to resolve all the suggestions except for the question about whether |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @tgross
Thanks for making these updates.
Aside from my in-line comments, this LGTM.
Co-authored-by: Seth Hoenig <seth.a.hoenig@gmail.com>
Co-authored-by: Radek Simko <radek.simko@gmail.com>
Resolved merge conflicts on the go.mod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - thanks for doing this!
One bugfix then we can merge this. I'd like to add some e2e tests but that can be done in a subsequent PR.
Co-authored-by: kmoe <5575356+kmoe@users.noreply.github.com>
The Nomad team has a fork of this project (https://github.com/shoenig/hc-install) with the command line interface implemented (and Vault added). We use this to install Consul and Vault as part of CI (ex github/workflows/test-core.yaml#L148-L149). Rather than having to keep up with release API changes, we'd like to upstream our work.
Fixes #13