Added a few doc strings to charm module #417
Conversation
Co-authored-by: chipaca <chipaca@users.noreply.github.com>
Co-authored-by: chipaca <chipaca@users.noreply.github.com>
Co-authored-by: chipaca <chipaca@users.noreply.github.com>
Co-authored-by: chipaca <chipaca@users.noreply.github.com>
Co-authored-by: chipaca <chipaca@users.noreply.github.com>
Co-authored-by: chipaca <chipaca@users.noreply.github.com>
Co-authored-by: chipaca <chipaca@users.noreply.github.com>
Co-authored-by: chipaca <chipaca@users.noreply.github.com>
Co-authored-by: chipaca <chipaca@users.noreply.github.com>
Co-authored-by: chipaca <chipaca@users.noreply.github.com>
ops/charm.py
Outdated
| """A base class for events that trigger because of a Juju hook firing.""" | ||
| """Events raised by Juju to progress a charm's lifecycle. | ||
|
|
||
| Hooks are callback methods of a Charm class (a subclass of |
There was a problem hiding this comment.
I don't think we are calling the observers of events 'Hooks' in the current codebase. (on.observe calls it just 'observer':
def observe(self, bound_event: BoundEvent, observer: types.MethodType):
)
It may be that we want observers of Juju hook events to be called hooks, but I think we want a different term here. At least, we should discuss to make sure everyone agrees on the terminology.
There was a problem hiding this comment.
@jameinel The class is called HookEvent hence my intention was to tie into the class name. Please do suggest what you would like me to change this sentence to.
There was a problem hiding this comment.
Hook event is a subset of events, specific to the events related to Juju hooks.
Certainly the hook itself isn't the callback. I would call the callbacks an event handler. (and possibly a hook event handler)
ops/charm.py
Outdated
| This event is triggered immediately after the first :class: | ||
| `ConfigChangedEvent` event. Callback methods bound to the event | ||
| should be used to ensure that the charm’s software is in a running | ||
| state. Note that the charm’s software should be configured so as |
There was a problem hiding this comment.
"in a running state" is not always true, but I could live with it.
(Namely, we haven't established other units in relationships so the application may not be ready to start). For example, you can't start the web frontend app image because you don't have the database details yet.
I would tend to say less that this happens immediately after config-changed and more that it happens after initial leadership/relation/config has been established, but before information about specific units in relations will be exposed.
Maybe that is being to abstract and making it hard to understand.
There was a problem hiding this comment.
How about saying something like :
The charm's software is in a running state, provided it has formed all the required relations.
@jameinel would this ^ be acceptable ?
There was a problem hiding this comment.
I still don't think it is true. At the point of start you have been given the rough shape of the model (relation-created events, config-changed, install, leader-elected). But you don't know about other units (you don't join the relation until after start, so you haven't gotten any relation-joined/relation-changed events.)
ops/charm.py
Outdated
|
|
||
| Juju fires this event every five minutes for the lifetime of the | ||
| unit. Callback methods bound to this event may use the | ||
| `add_metric` method of this class to send measurements to Juju. |
There was a problem hiding this comment.
That is my understanding, especially since we know this is broken with how Operator Framework is storing its state, and all the other charms would be breaking if this was being called.
ops/charm.py
Outdated
| remote unit is first observed by the unit. Callback methods bound | ||
| to this event may set any local unit settings that can be | ||
| determined using no more than the name of the joining unit and the | ||
| remote `private-address` setting, which is always available when |
There was a problem hiding this comment.
I wonder if this text is the mechanism that we want to explain 'joined'.
joined has to do with your units visibility of the other unit.
when the start hook finishes, your unit joins all of the relations for the application
which makes it start to be visible to related units on the other side, but also means that this unit starts to see other units. For example:
juju deploy a -n 2
juju deploy b -n 2
juju relate a b
if the real-world order is that
a/0 starts
b/0 starts
asynchronously
a/0 gets a relation-joined b/0
b/0 gets a relation-joined a/0
a/1 starts
asynchronously
b/0 gets a relation-joined a/1
a/0 gets relation-joined b/0
b/1 starts
a/0 gets relation-joined b/1
a/1 gets relation-joined b/1
# there may be deterministic ordering here, but async relative to the above
b/1 gets relation-joined a/0
b/1 gets relation-joined a/1
So it is true that "when a remove unit joins the relation" but it also happens that you get events for all the existing units when you join the relation.
There was a problem hiding this comment.
@jameinel If I understand correctly I need to remove the sentence "The event fires only when that remote unit is first observed by the unit." and replace it with something like "The event fires every time a new unit joins the relation. When this happens, each unit participating in the relation receives one such event, for every other unit of the other application participating in the relation" .
Kindly let me know if such a change would be more accurate and acceptable ?
| references to the departing remote unit, because there’s no | ||
| guarantee that it’s still part of the system; it’s perfectly | ||
| probable (although not guaranteed) that the system running that | ||
| unit has already shut down. |
There was a problem hiding this comment.
For good or evil, because this is the inverse of RelationJoined, you have the same symmetry to worry about. Given my above example:
juju deploy a -n2
juju deploy b -n2
juju relate a b
# relation-joined hooks run
juju remove-unit b/1
asynchronously
b/1 gets relation-departed a/0
b/1 gets relation-departed a/1
a/0 gets relation-departed b/1
a/1 gets relation-departed b/1
# if there is a peer relation
b/0 gets relation-departed b/1
b/1 gets relation-departed b/0
Now there is an env var that got added
JUJU_DEPARTING_UNIT
which was added in 2.7 or so to disambiguate for peer relations, because you can imagine that the same set of hooks are fired for
juju remove-unit b/0
and
juju remove-unit b/1
(in both of those cases on the peer relation, each unit sees the other unit 'departed'.)
There was a problem hiding this comment.
@jameinel thank you for pointing this out. Will it address your concern if I change the line "The :class: RelationDepartedEvent will fire once only." to "The :class: RelationDepartedEvent will fire once for every unit in the relation belonging to the other application. If not kindly suggest any edit you deem more appropriate.
| callback method bound to :class: `RelationDepartedEvent` event has | ||
| been run. If a callback method bound to this event is being | ||
| executed, it is gauranteed that no remote units are currently | ||
| known locally. | ||
| """ | ||
|
|
||
|
|
||
| class StorageEvent(HookEvent): |
There was a problem hiding this comment.
This is as far as I have gotten in this review.
There was a problem hiding this comment.
Awesome. Take a break. Have a beer. I am getting hungry too, and will call it a day soon but may hang out on IRC much later. I really appreciate you taking the time to do this.
Co-authored-by: chipaca <chipaca@users.noreply.github.com>
Co-authored-by: chipaca <chipaca@users.noreply.github.com>
Co-authored-by: chipaca <chipaca@users.noreply.github.com>
Co-authored-by: John Arbash Meinel <john@arbash-meinel.com>
Co-authored-by: chipaca <chipaca@users.noreply.github.com>
A Proposal for doc strings to charm module.