Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "no_rsc" [email protected]:tariq1890/distribution.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

requesting your review @caervs

Codecov Report

Merging #2926 into master will increase coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2926      +/-   ##
==========================================
+ Coverage   60.45%   60.47%   +0.01%     
==========================================
  Files         102      102              
  Lines        8001     7999       -2     
==========================================
  Hits         4837     4837              
+ Misses       2517     2515       -2     
  Partials      647      647

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3226863...8f9c809. Read the comment docs.

@dmcgowan @thaJeztah any opinions on this vs #2915? From where I stand reducing our dependencies is always a win and as @tariq1890 points out this is recommended in the letsencrypt readme.

Also want to state that this PR will lay down the path for go mod migration :). I'll have that up once this is merged.

See also: #2530

any opinions on this vs #2915?

I'm definitely not attached to #2915 - it was just to get rid of the fork, but (as Derek commented on that PR); that code needed replacing, so if this one does the job, I'm all in favour of that.

It looks like this PR is a duplicate of #2530 which was filed more than a year ago. It would be really great to see this merged. I am happy to get the PR review process going for this in case we are unable to revive the thread in #2530.

+1 this would be really nice to have. Currently dealing with some dependency hell related to needing to use the @dmcgowan fork of rsc.io/letsencrypt

Are documentation changes needed for this change?

I don't believe so. This PR intended to preserve any previous behaviour.

@caervs Did the e2e tests pass for this one?

Can I please get some response on this?

@tariq1890 yes turns out this change needs a few edits to work properly. Please:

• Add Prompt: autocert.AcceptTOS, to the manager as it requires a prompt function
• Set tlsConf.NextProtos = append(tlsConf.NextProtos, acme.ALPNProto) (required for verification with letsencrypt)
• Add a note to the config docs that the ca-certificates package is required when using letsencrypt to trust the letsencrypt cert

In the meantime we really need a second review to get this in. @manishtomar ptal. Thanks.

@caervs Thanks for your review comments. This is ready for another round of review.

@caervs Do we have tests to verify this?

@manishtomar I have manually tested with no cache in place and restarting with existing cache. I have not tested with existing cache from letsencrypt fork carrying over into new cache, but I think that'd be overkill. Worst case the workaround is either to remove the cache or change the cache directory.