Thank you all, for your feedback! @alex tracking
@adam 10-4 @charles | That all makes sense -the reason I wanted it in lib, was because I was leaving it open to the user of the layer to configure supervisor for their app(s). Would it be preferable to have the user specify a list of appnames as a config param, and iterate over them? On Thu, Jul 21, 2016 at 2:59 PM, Charles Butler < charles.but...@canonical.com> wrote: > Greetings James, > > +1 to Adams suggestion. > > A question about the readme examples: > > There's a given example where you're getting a Workers() object but it > isn't imported, or declared. Where did it come from? > > @when('myapp1.supervisor.available', > 'myapp2.supervisor.available')def run_workers(): > w = Workers() > w.start_tasks() > > > Emittersare really states, I would probably rename that header for > clarity. > > Looking into the code, it looks like a great start. Some minor nits though: > > > https://github.com/jamesbeedy/layer-supervisor/blob/master/lib/charms/layer/supervisorlib.py#L43 > > There's some host modifications going on in here I wouldn't expect to find > here. Really I would expect to see this bit handled in the reactive layer. > > Maybe place it around: > > https://github.com/jamesbeedy/layer-supervisor/blob/master/reactive/supervisor.py#L11 > > And try to keep host modifications to the reactive module, and keep the > library tidy for interfacing with supervisor and doing the redundant things > for the user when those come about like scaling workers, providing sensible > defaults for common values. > > I see quite a bit in here http://supervisord.org/configuration.html so > you're definitely on the right path :) > > > All in all its a great start, I look forward to seeing where this goes, > and would like to thank you for your continued contributions to the charm > and layer ecosystem. > > All the best, > > Charles > > > > On Thu, Jul 21, 2016 at 1:25 PM Adam Stokes <adam.sto...@canonical.com> > wrote: > >> looks good, one small nit pick, maybe change supervisorlib.py to just >> supervisor.py. that seems to be the current naming scheme of things >> >> On Thu, Jul 21, 2016 at 1:42 PM, James Beedy <jamesbe...@gmail.com> >> wrote: >> >>> A big thanks to those of you who gave feedback! I have made some >>> revisions based on the suggestions I received from a few of you (thank >>> you!), and would like to get a second round of feedback now the >>> modifications have been made -> [layer-supervisor]( >>> https://github.com/jamesbeedy/layer-supervisor). >>> >>> Thanks all! >>> >>> -- >>> Juju mailing list >>> Juju@lists.ubuntu.com >>> Modify settings or unsubscribe at: >>> https://lists.ubuntu.com/mailman/listinfo/juju >>> >>> >> -- >> Juju mailing list >> Juju@lists.ubuntu.com >> Modify settings or unsubscribe at: >> https://lists.ubuntu.com/mailman/listinfo/juju >> > -- > Juju Charmer > Canonical Group Ltd. > Ubuntu - Linux for human beings | www.ubuntu.com > Juju - The fastest way to model your service | www.jujucharms.com >
-- Juju mailing list Juju@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju