Comment 1 for bug 1073520

Hi Ante

I took a run through your charm - heres some feedback:

1) Error handling in hooks

Recommend that hooks be executed with -e to ensure that any errors during hook execution and propagated back to the end user through failed hooks; handle any non-zero exit codes as appropriate and let the rest bubble up.

2) 'provides' hooks

The charm provides three interfaces; however no hooks are present in the charm to handle these.

Not sure about the typing of the interfaces; they look the wrong way around - for example the type of the postfix interface is smtp rather then the type of the smtp interface being postfix.

That way when someone writes an exim charm it can fulfill the smtp interface in the same way.

3) exposing the service

Recommend that you open ports for potential exposure using 'open-ports' - otherwise in a secured environment the service won't ever be accessible remotely. They won't be exposed unless an admin does 'juju expose' so its opt in from an end user perspective.

4) empty defaults in config.yaml

default: "" is not really required - config-get returns nothing if nothing is provided.

5) relayhost config

I can see how this is a useful config when routing to an externally managed relayhost; but I guess this could also be a juju managed instance as well so maybe having a 'requires' relationship of type smtp could be an option as well?

Default configuration of local only gets a +1000 for good security.

This charm would be nicely complemented with a sub-ordinate charm that could be deployed alongside other services to provide satellite routing to a central mail relay/delivery location. You could use the information provided over such a relation to limit incoming mail acceptance to related service units only.

HTH

James