-
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
releases: support enterprise versions #148
releases: support enterprise versions #148
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.
Hi @colinodell
Thank you for sending the PR in.
This looks like a good start but there is one more aspect of enterprise releases which will need to be reflected before we can merge it in.
All enterprise products should come with two *.txt
files - EULA.txt
and TermsOfEvaluation.txt
in the archive.
After asking internally, I was advised that we'd need to be installing those files somewhere while installing the product binary to alleviate any legal concerns.
This implies some changes:
We can introduce LicenseDir
(string) which would be treated as required to let the user to specify where to place the files which contain licensing terms.
Since this brings us to a total of 3 fields related to enterprise, I wonder if it would make sense to instead turn this into a struct, e.g.
type LatestVersion struct {
Enterprise EnterpriseOptions
}
type EnterpriseOptions struct {
LicenseDir string // required
Meta string // optional
}
The code responsible for the installation would need to be updated to reflect that we need to install more than just a binary.
I see you updated releases.LatestVersion
but for consistency we should add support for enterprise products into other parts of the API as well, i.e.
Finally we should probably update the Readme.md
to include an example with an enterprise product so that folks know this is supported and how to do it.
I appreciate this may be much more work than you initially planned for, so one of the maintainers (like myself) can probably pick it up and finish the work later if you don't want/intend to. Just let us know.
Thanks for the detailed feedback! It is indeed more work than I had anticipated, but I'd be happy to take it on with your guidance :)
Currently, this PR installs those files into the
Adding support to the other
Is that still true? If so, I'd propose we avoid touching |
I forgot about that entirely. I believe that is still true, so yes we can avoid touching that one. 👌🏻
Instead of moving the files I think we could just unpack them directly to the appropriate path, i.e. modify this part of the function you mentioned and check for the filename there before deciding where exactly to copy the file from the archive, so we can avoid the additional I/O hc-install/internal/releasesjson/downloader.go Lines 169 to 181 in 850464c
That means we'll need to introduce one more argument to With all these changes we shouldn't need to worry about compatibility as we're not changing public interface at all nor changing behaviour of existing surface. |
46a0168
to
93622cc
Compare
I think I've made all the requested changes - this is ready for re-review! |
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 making all the changes, this is pretty close.
I left mostly minor suggestions and comments. Happy to merge once these are addressed.
I'm not entirely sure this is actually necessary or beneficial (from memory consumption perspective) but I'm keeping it as-is as it should do no harm to existing users who do not install enterprise products.
Thanks for your help and guidance here! 🎉 |
This pull request allows the installation of enterprise versions by supplying an
Enterprise bool
flag and an optionalEnterpriseMeta string
field as suggested in #15 (comment).According to #15 and #16, the ability to download enterprise versions was removed from the public release of this library. It seems this decision was likely made to prevent users from accidentally installing Enterprise versions. However, there are some valid cases folks have the need to automate the downloading of Enterprise versions:
With these proposed changes, users of this library can now opt-in to fetching the Enterprise versions as needed, without negatively impacting the ability for OSS users to continue using the OSS releases.