Code review comment for lp:~johnsca/charms/trusty/cf-cloud-controller/services-callback-fu

On 2014/05/27 18:06:56, benjamin.saller wrote:
> Not sure I love data_ready/data_lost. Still I feel like I'm bike
shedding and
> whit already reviewed this and was ok with the naming so I'll pass if
you don't
> like these better

> 'requires': [],
> 'complete': []
> 'incomplete': []

> another option is to keep with the naming we use in relations though
I'll admit
> 'joined' is a little hard to understand in this context, so maybe

> 'requires', 'changed', 'broken'

> I do think 'broken' is a good name, but it needs something that fits
> conceptually with it. At this point though I do think what you have is
> understandable and nicely documented.

Yeah, we talked about nomenclature quite a bit on Friday. Whit's point
was that our "dependencies" list was overloaded and actually serves two
purposes:

   1) Define what the service needs to operate that it can't get for
itself (relations, config data, etc)
   2) Collect all of that information into a unified view

Naming it "dependencies" or "requirements" makes that second aspect
non-obvious, and it would be best for the API to be, as much as
possible, self-documenting. Given that, "required_data" was the best we
could come up with, as it conveys both that this is a list of required
things, and that those things provide data somehow. This also plays
into the ability to, instead of having a semi-opaque StaticContext
class, you can now just drop a plain dict (or the results of config())
in there directly and it will do what you expect.

Then, I also realized that I wanted to separate the "ready" (or
"complete," or whatever) event from the "start" event, so that the
charms can just worry about defining what happens between everything the
service needs being available and spinning up the service itself,
without having to worry about the details of spinning it up by manually
specifying host.service_start every time, since 90% of charms should be
using Upstart or some other SysV-compatible service manager.

So, then we end up with the events of interest:

   * All of the requirements have been satisfied and the data is
available to configure and start the service
   * Something changed, and we no longer have complete information for
running the service

The names we chose, "data_ready" and "data_lost" seemed to capture those
ideas while matching up well to "required_data." Additionally,
"data_lost" specially calls out the fact that it doesn't need to be
triggered in the case that we're still working to get set up and we've
never had the service set up, so nothing needs to be done to tear it
down.

I'm not opposed to "complete" and "incomplete," or "complete" and
"broken." But I also think that it will not be difficult to change the
keys later in a backwards-compatible way (since we just look for one of
a set of keys, and it would be all encapsulated in fire_event).

So, my inclination is to leave it as-is until / if we get more feedback
that "required_data," "data_ready," and "data_lost" are confusing.

https://codereview.appspot.com/100680044/