-
Notifications
You must be signed in to change notification settings - Fork 37
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
Create submodules for issuers #67
Comments
Hi @SpeedyCoder! This issue made me confused because this is not how I thought it worked. So I created a test repo to see what would happen. The repo is available on https://github.com/johanbrandhorst/dep-test. It imports
Which is totally reasonable. The This convinced me about two things:
However, given /1, I think that this is not really an issue. Does that sound okay? |
To add some more context, I asked about this situation in #modules on gophers slack because I want to be a good maintainer and
Source: https://gophers.slack.com/archives/C9BMAAFFB/p1555012667093600 Now, I understand why /2 might be of some concern since you're not all that interested in cloning Vault if you're using CFSSL, but I think the argument right now is that
Furthermore, there is a significant long term cost to adding multi-modules to your repo. Versioning becomes much more complicated, and the risk of screwing it up is higher. Therefore, I am still hesitant to make this change. Please let me know if you disagree. |
Hey
First is the latest tagged version in this repo, second is my version with multiple submodules and last is a version based on a branch where I nuked the |
Great, thanks for proposing an alternative. I left some comments on the PR, but I think that's probably the sanest way forward right now. I'm hesitant to go multi-module without a better understanding of what that means in practice for me as a maintainer, having been recommended against it unless absolutely necessary. Lets see if we can get that PR merged 👍. |
One observation is you went from depending on a January version of I don't know if this explains some or all or none of what you saw in terms of reduced dependencies, but one thing that has been going on is the various |
Is your feature request related to a problem? Please describe.
With current setup users of the library need to pull dependencies for all issuers even though most of them will only use a single one.
Describe the solution you'd like
Create a submodule per issuer, then every user will only have to pull dependencies for the issuer they are using.
Additional context
This is quite simple to do I've done it in this fork https://github.com/utilitywarehouse/certify, I can make a PR if you want, but it's probably easier to set this up when you have access to the repo.
After switching to my fork with submodules I got the following change in
go.sum
file in a module that's using cfssl issuer https://gist.github.com/SpeedyCoder/1d9e6216cd5fc877ea4cb059faadc220(I also bumped dependencies, but the point is the number of lines removed...)
The text was updated successfully, but these errors were encountered: