Closed itsdalmo closed 6 years ago
Thoughts on this @lox?
Apologies, I've been sick! 😷Will review this tomorrow!
NP! Get well soon 🙌
Can we go with /cmd/lifecycled/main.go
so that go install ./cmd/lifecycled
does the right thing?
@lox - Thanks for the review! Have moved the binary, and feel confident that RFC3339
should work here - but we should test it in the wild before making a 3.0 release - are you up for that? 😄
edit: Just caught one error I had made; lifecycled.Config
accepts a SpotListenerInterval
, which was not being set in main.go
and therefore defaulted to 0 and crashed the binary on startup. Fixed now, and everything works just dandy with autoscaling termination notices (need you to test spot terminations if possible) 👍
What do you think @lox - is this ready to be merged?
Yeah, let's merge it. Sorry for the delay.
Based on the work in: https://github.com/buildkite/lifecycled/pull/50
I just decided to rewrite with inspiration from the PR above, to get a basic test suite up and running. In the process I figured it was prudent to do the following:
main.go
tocmd/main.go
so that we can use package names to ensure that we are testing the public interface (i.e. whats usable frommain.go
) in our tests.logrus.Logger
to pass around, so that we can useNullLogger
in the tests.log.Entry
to some listener and handler methods, so that we can propagate fields.Queue
methods (instead logging these from the listener itself).TerminationNotice
fromdaemon.Start()
so that we have a return value to test in our test suite. To not bloatmain.go
I decided to make aConfig
struct and have thelifecycled.New()
andlifecycled.NewDaemon()
functions do more of the boilerplate setup.With the above in place, I wrote three basic tests to validate the behaviour of the daemon, with the goal of ignoring implementation details as far as possible. So for future PR's etc we should be able to run tests to catch things like Queues not being cleaned up.
Possible improvements:
Let me know what you think 👍