Comment 7 for bug 1284738

Revision history for this message
Charles Butler (lazypower) wrote :

Francesco,

Thank you for the rapid turn around on this review. I really enjoyed going through the issues and observing your thought patterns in the changes to the charm source. This was a very nice addition to the review process that I don't normally see.

I only have 1 further nit pick to point out - the missing stop hook means that the syncope installation is left around after removal of the service. You'll want to remove or archive the syncope files (possibly to /mnt, which appears in other charms) in case of disaster recovery. That way we leave the machine tidy if someone were to destroy-service syncope and re purpose the server, or has deployed other services alongside that should not be destroyed.

Another interesting thing I ran into, which i'm not sure the charm is responsible for - when visiting the syncope URL i was unable to login. The service spat back that my session was invalid regardless of clicking sign out, using an in private browsing session, and fully clearing cache and restarting my browser. Is this due to a missing configuration value of a user/password combination?

Barring that modification and validation of service setup, you are very close to making it into the charm store. Thank you again for your rapid response to the charm review.

I'm going to move the status of this bug to incomplete. When you have completed the modifications and are ready for another review, move the status back to "Fix Committed" or "New" and someone will be along shortly to review your work.

Thanks again!