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

Create submodules for issuers #67

Closed
SpeedyCoder opened this issue Apr 11, 2019 · 5 comments
Closed

Create submodules for issuers #67

SpeedyCoder opened this issue Apr 11, 2019 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@SpeedyCoder
Copy link
Contributor

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...)

@SpeedyCoder SpeedyCoder added the enhancement New feature or request label Apr 11, 2019
@johanbrandhorst
Copy link
Owner

johanbrandhorst commented Apr 11, 2019

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 github.com/johanbrandhorst/certify and github.com/johanbrandhorst/certify/issuers/cfssl. I ran go mod init followed by go mod tidy. The go.mod file looks like this:

module github.com/johanbrandhorst/dep-test

go 1.12

require github.com/johanbrandhorst/certify v1.1.5

Which is totally reasonable. The go.sum file looks much bigger. I don't understand this, and it includes loads of things that the repo has no right of importing. This confused me. However, I ran go mod vendor to see what the actual dependencies were, and as you can see in the repo, none of the other issuer packages are actually in the vendor folder.

This convinced me about two things:

  1. Importing the cfssl issuer does not actually import any packages that it doesn't need.
  2. I have no idea how the go.sum file works.

However, given /1, I think that this is not really an issue. Does that sound okay?

@johanbrandhorst
Copy link
Owner

johanbrandhorst commented Apr 11, 2019

To add some more context, I asked about this situation in #modules on gophers slack because I want to be a good maintainer and do the right thing. The following seems to happen when you import certify in your modules repo:

  1. It shallow clones the certify repo to get its go.mod file.
  2. It shallow clones any dependencies listed in the certify go.mod file, to get their go.mod files.
  3. After resolving the dependency tree, it'll then finish cloning the actual files it needs.

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 it doesn't really matter, and in the future, when we have Go modules proxies, it will matter even less. Please see https://gophers.slack.com/archives/C9BMAAFFB/p1554997171014900 for more information. A quote from the guy on the go team working on modules:

It downloads at least their go.mod files. Without a GOPROXY set, that requires -cloning- fetching the repo, but with a GOPROXY it's literally just the one tiny file. (edited)

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.

@SpeedyCoder
Copy link
Contributor Author

Hey
Thanks for looking into this. The ,modules stuff is quite interesting. My main issue is that anything using this library pulls a ton of dependencies in CI. I've tested how much dependencies I get in various scenarios using your example from https://github.com/johanbrandhorst/dep-test. I ran go mod download in the latest golang docker container withhout GOPROXY set and then with it set to GOPROXY=https://goproxy.io and then looked at the size of the /go/pkg/mod/ directory, cleaning it up between different runs and this is what I got

GOPROXY unset GOPROXY=https://goproxy.io
github.com/johanbrandhorst/certify v1.1.5 2.6G 1.6G
github.com/utilitywarehouse/certify v1.2.0 212M 110M
github.com/utilitywarehouse/certify @tidy-mod 565M 373M

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 go.mod and go.sum files and then repopulated them by running tests and go mod tidy. The last one is quite interesting as it suggests there is quite a lot of stuff that actually isn't used anywhere in the go.mod and go.sum files. I've made a PR #68 with the changes from the last scenario, so it would be nice to get that or something similar merged if nothing else.

@johanbrandhorst
Copy link
Owner

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 👍.

@thepudds
Copy link

thepudds commented Apr 12, 2019

One observation is you went from depending on a January version of golang.org/x/tools to a March version of golang.org/x/tools

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 golang.org/x repos have eliminated an enormous number of dependencies over last couple of months. Things like golang.org/x/tools or golang.org/x/build could end up pulling in things like opencensus and prometheus which in turn would pull in an even wider set of dependencies, but that has been cleaned up substantially in the golang.org/x repos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants