Code review comment for lp:~jimbaker/pyjuju/new-hook-semantics-5-docs

Very good changes overall, thanks Jim.

We'll need another round:

[1]

17 +A formula is deployed as a service, potentially multiple times. Each

This is very open and perhaps even misleading. Please take this specific
sentence off. The rest of the paragraph should give better background.

[2]

+The `metadata.yaml` file, at the root of the formula definition,

s/definition/directory/

[3]

- and `peers` relations (note: that's not yet supported).
(...)
- that the relation is required (note: that's not yet supported).

Please add back both unsupported notes. All documentation which is not
under drafts should reflect what is implemented and may be trusted upon,
or declare explicitly what is not yet in place.

130 + At this time, Ensemble does not enforce the `limit` and
131 + `optional` constraints, but Ensemble will do so in the future.

The note really has to be closer to the explanation. Someone reading about
the field to tweak a formula will not read the whole text to see what is
supported or not.

[4]

188 +changes on its underlying machine, and change its own relation
189 +settings. Changes to relation settings then trigger further events
190 +that hooks can respond to. The following section on hook tools

s/its own relation settings/the relation settings/ (the subject of the
sentence is "the hook").

Also, please drop the sentence starting at "Changes to relation". You're
describing what happens from the perspective of an individual unit. For
an individual unit, changing relation settings will not trigger its own
hooks to execute again.

[5]

194 +the desired hook name under the ``hooks/`` directory of the formula
195 +bundle or directory

s/bundle or directory/directory/

Conceptually, we use the bundle term to reference to a file, so it can't
have directories.

[6]

195 +bundle or directory. Ensemble then executes the hook without command
196 +line arguments when the corresponding event occurs.

Please use this in place of the latter sentence:

  Ensemble will execute the hook based on its file name when the corresponding
  event occurs.

Note that in general we don't have to document the lack of something. If it's
not documented to exist, it's fine to not exist.

[7]

198 +Each hook is optional. Not including a corresponding executable in the

s/Each hook is/All hooks are/

[8]

213 + * **stop** - Runs when the service unit is stopped. This hook will run
214 + after any established relations are broken.

Let's take the chance to fix the pre-existing ambiguity in the latter sentence:

  If relations exist, they will be broken and the respective hooks called before
  this hook is called.

[9]

+ 1. A remote service unit joins the relation.

Let's complement:

    1. A remote service unit joins the relation, right after the
       relation-joined hook was called.

[10]

229 + This hook is the most common hook for a formula to implement. It
230 + enables service units to handshake and otherwise negotiate their
231 + interaction, especially with respect to scaling up and down
232 + service deployments.

This doesn't feel very helpful. It's taking assumptions about how the user
should use the hook in his formula rather than explaining what the hook
does so users can figure how they can use it. It's also not true that it's
"specially with respect to scaling up and down", nor that this is the most
common hook (install would be the most common, I think, but that's not
important).

Please replace with this simpler version:

    This hook enables the formula to modify the service unit state (configuration,
    running processes, or anything else) to adapt to the relation settings of
    remote units.

[11]

238 + the most sense. Upon that happening, the HAProxy in its
239 + ``<relation-name>-relation-changed hook`` can then change its own

s/<relation-name>-relation-changed/server-relation-changed/, since this is an example.

[12]

242 + This interaction is loosely coupled. The HAProxy formula doesn't
243 + need to know what services it works with. The other services don't
244 + need to know they're working with HAProxy. They simply need to
245 + agree on a common set of relation settings that allow them to be
246 + integrated.

This is within the documentation of relation-changed hook, so it's not a good
place to define how relations work conceptually. We also need some text to
explain that in a generic way, rather than introducing the idea with an example.

Please remove this, and then put the following text somewhere sensible place
inside the Hooks section:

"""
Note that the coupling between formulas is defined by which settings are
required and made available to them through the relation hooks, and how these
settings are used. Those conventions is what defines what the relation
interface really is, and the interface name in the metadata is simply a way
to refer to them and avoid that incompatible conversations be attempted.
Keep that in mind when designing your formulas and relations, since it's
a good idea to allow the implementation of the formula to change and be
replaced with alternative versions without changing the relation conventions
in a backwards incompatible way.
"""

[13]

261 + * **<relation name>-relation-broken** - Runs when a service is
262 + removed from a relation, not just a service unit. This event

This is different from what was previously documented, and I don't think
it's correct. Please read the original documentation, and maybe the
implementation as well.

[14]

345 +a non-zero status code. Such changes will then trigger further hook
346 +execution, through the **<relation name>-relation-changed** hook. This
347 +mechanism enables a general handshaking mechanism for service units to
348 +coordinate.

s/hook execution/hook execution in the remote unit(s)/

s/handshaking/communication/