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 Review" or "Fix Committed" and someone will be along shortly to review your work. Thank you again for the time vested and effort to come. We really appreciate it and look forward to this IDM solution.