New Apache Syncope Juju Charm

[Francesco Chicchiriccò]

New Juju Charm about Apache Syncope:

https://code.launchpad.net/~francesco-chicchiricco/charms/precise/syncope/trunk

[Francesco Chicchiriccò (francesco-chicchiricco)]

Adding a fake comment to trigger notification to charmers.

[Marco Ceppi (marcoceppi)]

Hi Francesco, you need to make sure the bug is assigned to you and your branch is linked in order to get added to the review queue. I've gone ahead and done this for you and verified your charm is now in the review queue. A charmer will be by shortly to review the charm and provide any feedback if necessary.

Thanks for the submission!

[Francesco Chicchiriccò (francesco-chicchiricco)]

Hi Marco, it seems I've missed some basic steps here: sorry and thanks for your support.
Regards.

[Charles Butler (lazypower)]

Greetings Francesco,

Thank you for taking the time to work through charming up Apache Syncope! I'm excited to review your work that has landed after we first met in the charm school. I've reviewed the syncope charm, and I have the following notes:

When running `charm proof` I received the following output:
I: relation website has no hooks

If this is designed to hook into a load balancer, the website-relation-joined hook should set the ip address of the unit and associated port of the underlying service so consuming charms like HAPROXY can set up proper routing. I see that scale out usage is still pending in the README, and assume this is being worked on.

in config.yaml you have a commented out configuration option for jee-container - this looks like a leftover from development and triggered a failure during charm deployment when testing. I uncommented the block and redeployed, and syncope deployed as expected.

The lions share of the charm logic is in the install hook - however with configurable options like your J2EE environment (say we deploy on glass fish and change to jboss) - we have an immutable environment. I would like to see these components moved to config-changed, or update config-changed to call the installation hook to ensure these configuration options when changed are reflected on the underlying service.

I'm pleased to see that you have implemented md5 sum checking of remote file downloads -

Knit Picks:

On line 101 of the installation hook, there is a 30 line heredoc inline. Typically we ask that these types of heredocs be moved either to their own file to cut down on hook length for ease of review, or be written in a template technology like jinja2, cheetah, or whatever flavor you are comfortable with for template generation.

db-relation-broken, db-relation-departed, db-relation-joined hooks are empty. If these hooks are not going to be taking any active action on the host, they should be removed from the charm. Juju by default skips non-existent hooks.

By default I was unable to assume a working deployment using the default constraints. I urge you to add the required constraints for host deployment to the readme example deployment workflow so its apparent to users unfamiliar with the service that this will require specific machine bare-minimums to run.

Example:

$ juju deploy syncope --constraints mem=2G

After successful deployment and I was able to validate tomcat was running, I was unable to view the syncope environment. I urge you to add the steps to view the syncope interface without jumping through hoops to create and add a tomcat user to the tomcat config, cycle the services, etc. It should be apparent to the user that they have to visit a specific URL/Port on the installed instance.

This is a great start to the Syncope service, and bringing IDM to all users of Juju. You've really cut down a lot of the headache of setting up Identity Management with the groundwork you've laid out here. Barring some modifications your charm is well on its way to making it into the charm store.

I'm going to set the status of this bug to incomplete. When you are ready for another code review simply set the status to "Needs...

[Francesco Chicchiriccò (francesco-chicchiricco)]

I've opened and fixed issues for all observations above in the GitHub repository (see [1] for details); ready for next review.
Thanks.

[1] https://github.com/Tirasa/SyncopeJujuCharm/issues?milestone=1&page=1&state=closed

[Charles Butler (lazypower)]

Thank you for the update Francesco. I'm pulling your github code now for the review and will report my findings shortly

[Charles Butler (lazypower)]

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!

[Kapil Thangavelu (hazmat)]

On Wed, Mar 5, 2014 at 5:41 PM, Charles Butler <<email address hidden>
> 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.
>

just to clarify a minor point.

machines cannot be repurposed in core without manual placement, a machine
is basically marked dirty once its been used by a charm. where as juju will
use any allocated unused pristine machines to satisfy new requests for
machines.

its still good practice to implement a stop hook

[Francesco Chicchiriccò (francesco-chicchiricco)]

I have added the stop hook [1] and taken a close look to the cause of not working deployment [2].

As you might read from [2], I think it is just a bug on PostgreSQL side, hence I have added some specific workaround instructions on README.

Final note: I usually rsync any change from GitHub repository [3] to the branch related to this issue [4], while I see you've been pulling the code from GitHub. I am naturally fine with this but I was wondering: what is the best practice?

Regards.

[1] https://github.com/Tirasa/SyncopeJujuCharm/issues/16
[2] https://github.com/Tirasa/SyncopeJujuCharm/issues/17
[3] https://github.com/Tirasa/SyncopeJujuCharm
[4] https://code.launchpad.net/~francesco-chicchiricco/charms/precise/syncope/trunk

[Charles Butler (lazypower)]

Francesco,

I've re-reviewed the submissions and I'm pleased to say you've passed my +1 review.

In response to the keeping different repositories in sync - thats something I have struggled with myself. I don't have a good answer on what a preferred practice is, but I think it would be a safe recommendation to:

Keep your upstream in GitHub, and as you reach release candidates for the charm store - you then rsync that tag over to bzr and push to launchpad. It's really the maintainers choice on how to do so. It may be a good idea to email the list - <email address hidden> and ask the community at large if they have any tips for you. I know you're not pioneering alone in this endeavor.

Thanks for the excellent work Francesco. I'm going to leave the bug as is, and a juju charmer should be along shortly with another review for you.

All the best!

[Marco Ceppi (marcoceppi)]

Francesco,

I've given this a final review prior to official promulgation. As far as I can see there are not major blockers for this charm so I've promulgated the charm. I'll be opening bugs against the promulgated charm to track those.

Thanks for your attention and revisions to the charm and seeing it through to completion!