I've pushed up some changes.

On Fri, Jun 29, 2012 at 11:09 AM, Robert Collins
<email address hidden> wrote:
> On Fri, Jun 29, 2012 at 10:42 AM, Clint Byrum <email address hidden> wrote:
>> Hi Robert, thanks for submitting. Obviously we would require at least
>> some kind of commitment to maintain this charm long term or we can't
>> accept it into the charm store. That means listing yourself as
>> maintainer and being responsive to bug reports and questions.
>
> So, I'm happy to list myself as maintainer (how do I do that), and
> respond to bug reports and q's. I just can't commit to /doing/
> anything about them until/unless we were to use it in production...
>
>> Anyway, ignoring that, here is a review of the content of the charm:
>>
>> ** This is great, and I really love the detailed instructions in README
>>
>> 1. git:// urls from github are not safe, and can be MITM attacked. You
>> can however use the https urls and at least be covered for MITM with
>> that.
>
> Will do.

Done.

>> 2. start should indeed start anything that needs to be running at the
>> time it is run, so it should inspect system state and start any daemons,
>> even if they are started elsewhere in the charm.
>
> Is there a stock way to factor this out to avoid duplication?

Don't know how to:
12:24 < lifeless> how do you interrogate a specific relation from a charm hook?
12:24 < lifeless> (I want to see if the zk relation is setup in
opentsdb in the start hook)
12:24 < imbrandon> relation-get ?
12:25 < lifeless> so, in the zookeeper-relation-changed hook, I do
zk_host=`relation-get private-address`
12:25 < imbrandon> yup
12:25 < lifeless> but that is missing the context in the start hook.
12:25 < lifeless> isn't it? Or am I misunderstanding teh whole thing
12:26 < imbrandon> right, you only have access to it in the relation* hooks
12:26 < lifeless> so thats my quesiton
12:26 < imbrandon> ahh as far as i konw you dont
12:26 < imbrandon> or cant
12:27 < lifeless> SpamapS: ^ your review, how can I do this thing?
12:30 < imbrandon> but yea as far as the relation stuff , i dont think
there is a way iirc, other than some questionable measures
12:30 < imbrandon> like using the juju charm from the cli of a hook
12:30 < imbrandon> or similar


>> 3. stop maybe needs --oknodo since opentsdb might not be running at the
>> moment stop is called.
>
> ok.

Done.

>> 4. zookeeper-relation-changed: its only psychological, as the change
>> doesn't actually happen until the hook exits successfully, but open-port
>> shoudl come after the port that you're opening is serviced, since it is
>> meant to be used as a way to say "this is now ready" not just a marker
>> for what to expose.
>
> Sure, cando.

Done.

>> 5. rather than calling start-stop-daemon directly, you should arrange
>> for the daemon to start up if the box is rebooted. An upstart job
>> running basically the same command would be good
>
> Is there something I can cargo cult? Why doesn't juju take care of
> restoring state when the box is restart? I mean - it knows where we
> are meant to be...

Not sure how to (and this really feels like something that should be
an abstracted service not hand crafted).

>> 6. Do you mean to run tsdb as root? It will run as root right now, and
>> thats usually not cool.
>
> To a degree 'meh'. Packaging is where I would really prefer all that
> detail to go, but its not packaged, and the investment in doing that
> is (again, for now) disproportionate to the benefits. I'll add an
> adduser call to install, and sudo to the the user at appropriate
> places.

Done.

I haven't retested yet, I'll do that later - looking for feedback on
the undone bits above.