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

Imports & Dependencies #100

Closed
jehiah opened this issue Feb 6, 2016 · 8 comments
Closed

Imports & Dependencies #100

jehiah opened this issue Feb 6, 2016 · 8 comments

Comments

@jehiah
Copy link
Contributor

jehiah commented Feb 6, 2016

I sweat dependency trees, and really enjoy light libraries. One of the initial appeals to me of lego over hlandau/acme was how light this project was in terms of dependencies to accomplish what i needed. ❤️

Unfortunately, with the current code structure, the addition of support for multiple DNS providers to solve the dns-01 challenge has increased the dependency tree 🙈.

To import github.com/hlandau/acme the current dependency tree you need to support is

$ go list -f '{{join .Deps "\n"}}' |  xargs go list -f '{{if not .Standard}}{{.ImportPath}}{{end}}'
github.com/crackcomm/cloudflare
github.com/miekg/dns
github.com/mitchellh/goamz/aws
github.com/mitchellh/goamz/route53
github.com/square/go-jose
github.com/square/go-jose/cipher
github.com/vaughan0/go-ini
github.com/weppos/dnsimple-go/dnsimple
golang.org/x/crypto/ocsp
golang.org/x/crypto/sha3
golang.org/x/net/context

Of these, I'd love to skip 6 of them (vaughan0/go-ini is a sub dependency of mitchellh/goamz/route53), so I'd like to propose two options for refactoring the code structure to make these dependencies separate from the main acme package.

a) Move all providers into a single xenolf/lego/providers package
b) Move per-challenge pacakges (ie: xenolf/lego/providers/dns01 xenolf/lego/providers/http01 etc)
c) move to per-provider packages (ie: xenolf/lego/providers/dns/cloudflare, xenolf/lego/providers/dnsimple, ...)
d) some combination of the above?

(Note: i'm proposing structure, not specific naming). Since i personally don't care about the DNS providers, 2 and 3 are equal for me. I imagine others might feel differently, and the ideal case might change when more options w/ dependencies are added (like say #28).

The key is that the acme package doesn't import these packages (because that makes them all a dependency), the cli code would import them and set them as appropriate on the Client. Depending on the approach, the default http and tls-sni providers may or may not be moved.

thoughts @xenolf ?

@xenolf
Copy link
Member

xenolf commented Feb 6, 2016

Yes... I also do not feel too good with all these dependencies directly in the acme package.

My thought was that I first wanted to get the feature stable and then do a clean up to structure it in a way I feel comfortable with.

As we progress on DNS-01 I think it is a good time to start thinking about this.

@xenolf
Copy link
Member

xenolf commented Feb 6, 2016

That being said... I am in favor of packages per DNS provider and just having the manual provider in the acme package itself as it does not add a dependency but allows for completion of the challenge.

@mholt
Copy link
Contributor

mholt commented Feb 6, 2016

I actually got an email from someone packaging Caddy for Debian and since Caddy uses the acme package, and mitchellh/goamz was one of them that was questioned as why it was being used. Also the miekg/dns package is cool but it's huuuge.

+1 to refactoring.

@jehiah
Copy link
Contributor Author

jehiah commented Feb 6, 2016

related to dependencies, was there a specific motivation for using mitchellh/goamz over https://github.com/aws/aws-sdk-go?

@xenolf
Copy link
Member

xenolf commented Feb 7, 2016

ping @janeczku

@janeczku
Copy link
Contributor

janeczku commented Feb 7, 2016

+1 to the refactoring

related to dependencies, was there a specific motivation for using mitchellh/goamz over https://github.com/aws/aws-sdk-go?

Mostly that it seemed overkill to import such a huge library for basically just a single API method:

awsvsgoamz

@xenolf
Copy link
Member

xenolf commented Mar 14, 2016

@jehiah #144 was merged and it removed most of the dependencies from the ACME package.
The dependency tree looks like this now:

github.com/miekg/dns
github.com/square/go-jose
github.com/square/go-jose/cipher
golang.org/x/crypto/ocsp
golang.org/x/net/publicsuffix

We could not get rid of the github.com/miekg/dns dependency for the time being as it is needed to do the DNS checks for the validation records and I was unable to find a proper solution to move this check out of the challenge itself.

@jehiah
Copy link
Contributor Author

jehiah commented Mar 14, 2016

@xenolf this is fantastic. I think that resolves this issue!

@jehiah jehiah closed this as completed Mar 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants