Code review comment for ~graylog-charmers/charm-graylog:feature/multi-version

On Wed, Sep 4, 2019 at 1:58 AM Paul Goins <email address hidden> wrote:

> > diff --git a/lib/charms/layer/graylog/__init__.py
> b/lib/charms/layer/graylog/__init__.py
> > index e69de29..38bffbe 100644
> > --- a/lib/charms/layer/graylog/__init__.py
> > +++ b/lib/charms/layer/graylog/__init__.py
> > @@ -0,0 +1,39 @@
> > +from charmhelpers.core.hookenv import log
>
> Purely stylistic; don't consider this a -1. (At least not from me, as I'm
> somewhat new to reactive charms.)
>
> Per charms.reactive docs, this type of layer lib file seems intended for
> base layers rather than charm layers. And I do see similar helper
> functions directly in the reactive/graylog.py module.
>
> Is the intent for this to be reused by other charms? Or simply as a "lib"
> module separate from the handlers?
>

The latter; this module is meant to be helpful funcs for graylog that
aren't reactive. The helpers you see in reactive/gl.py should also be in
this lib. Ideally, only @reactive funcs would appear in reactive/*.py, but
we didn't start that way, so that's not how it is today. That said, we
don't have to keep going down the wrong road. There's an opportunity for
better organization in this charm, but that's beyond the scope of the v3
changes proposed here.

> diff --git a/reactive/graylog.py b/reactive/graylog.py
> > index 0d64f26..4a718bd 100644
> > --- a/reactive/graylog.py
> > +++ b/reactive/graylog.py
> > @@ -55,15 +67,56 @@ rotation_strategies = {
> > }
> >
> >
> > +@when_not('snap.installed.graylog')
>
> I've got a little concern here. If I understand the charms.reactive docs
> correctly; it doesn't seem that there's a guaranteed order that multiple
> matching handlers get executed in. (I'm relatively new to reactive charms
> so I may be wrong; correct me if so!)
>
> I think there's no guarantee here whether the snap layer's install()
> function or this install_graylog() function will be run first.
>
> While it feels kind of dirty, I think one way to guarantee this runs after
> the snap layer w/o requiring changes to it would be to add a
> @when('snap.installed.core') decorator, as the snap layer will set that
> once it runs its install() function. (I'd prefer a clearer flag that the
> snap install handler has run, but this should work I think.)
>

The problem with @when('snap.installed.core') is that once it's set, it's
set forever. That means every hook will trigger that handler until the end
of time. Yes, it's not a big handler -- we'd call "snap install graylog"
on the system and it would come back with "hey, it's already installed".
Still, it's burning cycles needlessly. I understand your concern, but snap
is smart enough that we don't actually have to wait for 'core'. If you do
a 'snap install hello-world' and 'core' isn't there, snapd will bring it
(and any other prereqs) down first.

It's safe to do @when_not(snap.installed.graylog) to guard the install
regardless of prereqs like 'core' being installed. In fact, layer-snap's
install function will set 'snap.installed.core' if it's not already:

https://git.launchpad.net/layer-snap/tree/lib/charms/layer/snap.py#n60