Add an issue_cert method to AcmeIssuingService

[mithrandi]

Fixes #76.

---

This change is

[codecov-io]

Current coverage is 99.94% (diff: 100%)

Merging #78 into master will increase coverage by <.01%

@@             master        #78   diff @@
==========================================
  Files            25         25          
  Lines          1818       1897    +79   
  Methods           0          0          
  Messages          0          0          
  Branches        163        171     +8   
==========================================
+ Hits           1817       1896    +79   
  Misses            1          1          
  Partials          0          0          

Powered by Codecov. Last update 7156fe1...7f39271

[jerith]

Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions.

---

src/txacme/service.py, line 75 at r1 (raw file):

    _waiting = attr.ib(default=attr.Factory(list), init=False)
    _issuing = attr.ib(default=attr.Factory(dict), init=False)

Why the extra init=False here? Is it something that should really have been there all along?

---

src/txacme/service.py, line 163 at r1 (raw file):

        d = Deferred(lambda _: d_issue.cancel())
        if server_name in self._issuing:
            d_issue, waiting = self._issuing[server_name]

Do we need d_issue here? Nothing in this branch (or after) uses it.

---

src/txacme/testing.py, line 83 at r1 (raw file):

        self._waiting = []
        if controller is None:
            controller = FakeClientController()

We currently only create a FakeClient in one place and we pass in a controller. Would it make sense to simplify this constructor and either always create the controller internally (and grab it from the client where we need it) or always pass one in?

---

Comments from Reviewable

[mithrandi]

Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions.

---

src/txacme/service.py, line 75 at r1 (raw file):

Previously, jerith (Jeremy Thurgood) wrote…

Why the extra init=False here? Is it something that should really have been there all along?

It's an incompatible change to the signature of **init** but I think being able to pass in a value for `waiting` makes no sense and is very confusing. I think when I wrote this code originally, `attrs` hadn't yet added support for `default=...` combined with `init=False`, so that may be why I originally didn't have it that way.

---

src/txacme/service.py, line 163 at r1 (raw file):

Previously, jerith (Jeremy Thurgood) wrote…

Do we need d_issue here? Nothing in this branch (or after) uses it.

The code that uses it is in fact located _before_ this branch; it's the lambda passed as the canceller two lines up. That's admittedly a little confusing, but I didn't want to duplicate the instantiation of the `Deferred` and both branches need it.

---

src/txacme/testing.py, line 83 at r1 (raw file):

Previously, jerith (Jeremy Thurgood) wrote…

We currently only create a FakeClient in one place and we pass in a controller. Would it make sense to simplify this constructor and either always create the controller internally (and grab it from the client where we need it) or always pass one in?

We actually create a `FakeClient` in three places (by my count: `test_endpoint`, `test_client`, and the integration tests), and two of them don't currently need the controller functionality.

Grabbing the controller from the client is problematic, because the real client of course doesn't have such an attribute; relying on "testing only" APIs makes it easier to accidentally introduce a bug by writing code that only works with the test double, but not the real implementation. That's the usual argument against doing this, anyway...

Always passing one sounds reasonable, though; explicit is better than implicit, or something?

---

Comments from Reviewable

[jerith]

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

src/txacme/service.py, line 75 at r1 (raw file):

Previously, mithrandi (Tristan Seligmann) wrote…

It's an incompatible change to the signature of init but I think being able to pass in a value for waiting makes no sense and is very confusing. I think when I wrote this code originally, attrs hadn't yet added support for default=... combined with init=False, so that may be why I originally didn't have it that way.

OK, thanks for the explanation.

---

src/txacme/service.py, line 163 at r1 (raw file):

Previously, mithrandi (Tristan Seligmann) wrote…

The code that uses it is in fact located before this branch; it's the lambda passed as the canceller two lines up. That's admittedly a little confusing, but I didn't want to duplicate the instantiation of the Deferred and both branches need it.

Maybe a brief comment or something (either here or above) would help?

---

src/txacme/testing.py, line 83 at r1 (raw file):

Previously, mithrandi (Tristan Seligmann) wrote…

We actually create a FakeClient in three places (by my count: test_endpoint, test_client, and the integration tests), and two of them don't currently need the controller functionality.

Grabbing the controller from the client is problematic, because the real client of course doesn't have such an attribute; relying on "testing only" APIs makes it easier to accidentally introduce a bug by writing code that only works with the test double, but not the real implementation. That's the usual argument against doing this, anyway...

Always passing one sounds reasonable, though; explicit is better than implicit, or something?

I didn't find those other places. Given that they exist, though, the slight extra complexity involved in making this optional is almost certainly worthwhile.

---

Comments from Reviewable

[mithrandi]

Review status: 3 of 4 files reviewed at latest revision, 1 unresolved discussion.

---

src/txacme/service.py, line 163 at r1 (raw file):

Previously, jerith (Jeremy Thurgood) wrote…

Maybe a brief comment or something (either here or above) would help?

Done.

---

Comments from Reviewable

[jerith]

---

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable