Add clock mock plumbing throughout the project #408
Conversation
| @@ -8,6 +8,7 @@ import ( | |||
| "os" | |||
| "strconv" | |||
|
|
|||
| "github.com/jmhodges/clock" | |||
There was a problem hiding this comment.
| "github.com/jmhodges/clock" | |
| "github.com/jmhodges/clock" | |
| @@ -34,6 +36,7 @@ type CAImpl struct { | |||
| log *log.Logger | |||
| db *db.MemoryStore | |||
| ocspResponderURL string | |||
| clockSource clock.Clock | |||
There was a problem hiding this comment.
Here and throughout, let's rename clockSource to clk, to match Boulder.
| if clockSource == nil { | ||
| ca.clockSource = clock.New() | ||
| } |
There was a problem hiding this comment.
Two comments:
- Here and throughout, honestly I think it's fine to leave this check/default off. The clk can just be a required argument and pebble can just panic if it isn't provided. Just like it would if the db argument was nil.
- But if you really do want to keep it optional, do this check/default before the creation of the CAImpl on line 349.
| db := db.NewMemoryStore() | ||
| ca := ca.New(logger, db, c.Pebble.OCSPResponderURL, alternateRoots, chainLength, c.Pebble.CertificateValidityPeriod) | ||
| va := va.New(logger, c.Pebble.HTTPPort, c.Pebble.TLSPort, *strictMode, *resolverAddress) | ||
| clockSource := clock.New() |
There was a problem hiding this comment.
Based on your PR description (you mention "breaking the external API") it sounds like you're using Pebble as a library, rather than as a standalone binary? In that case, your changes here make sense.
But if we're going to go to the trouble of giving Pebble full fake clock support, we should give it to everyone, including people who use Pebble as a binary (which I think is most users of Pebble).
To that end, let's replace this line with something a bit more like this, so that the fake clock can be initialized from an environment variable if someone wants to, or defaults to the system clock otherwise.
|
Thanks for the review, I'll address your comments when I have some free time. |
Hey, it appears that the repo included github.com/jmhodges/clock as a dependency at some point, but it was removed (#231).
As I was trying to integrate pebble into our integration tests, I ran into the issue of the lack of the clock mocks, which was a dealbreaker. I reintroduced the dependency here, unfortunately it breaks the external API and it's probably not done the way you, the maintainers, would approach it. I'm open to spending some time on fine tuning this PR to better match the rest of the codebase stylistically. This was the bare minimum of changes I had to make to make it all work.