Code review comment for lp:~sinzui/charms/precise/juju-gui/nagios

Revision history for this message
Benji York (benji) wrote :

Thanks for wiring this up. Monitoring is essential.

Given that this MP doesn't have Reitveld links, I assume you didn't use
lbox to propose it. Despite it's many annoyances, lbox is the
prescribed way of proposing and landing GUI branches. The most
important thing it does is to run "make check" before proposing a
branch. Please manually run that command before landing. Thanks.

> I decided to include just the nrpe module and its dependencies in the
> charm's tree.

It's a shame that our policies push us toward doing things like copy
chunks of code from outdated dependencies, but I can't proffer anything
better.

In check-front-page.sh (line 31 of the diff) shouldn't it be "sites-enabled"
instead of "sites-available"?

Also, I suggest using "https://127.0.0.1:443/juju-ui/version.js" for ADDRESS
and "jujuGuiVersionInfo" for LIFE_SIGN. Being code, that is less likely
to change because of a UI/UX redesign and we can write a test for it
that ensures if it does change a test named "testMonitoringEndpoint"
will fail. Oh, and we need that test in this branch too.

Once the above are addressed, this will be ready to land.

review: Approve

« Back to merge proposal