New charm: postfix

[José Antonio Rey]

Postfix charm: https://code.launchpad.net/~jose/charms/precise/postfix/trunk

[Robert Ayres (robert-ayres)]

Thanks for submitting your charm. Nice to see a popular e-mail server such as Postfix become a charm :)

Please see the following bugs/comments.

Bugs
*hooks/install, initial variables are all prefixed with '$' which causes command execution rather than variable assignment.

*hooks/install, debconf-set-selections config is missing types for two entries ('postfix/bad_recipient_delimiter', 'postfix/not_configured') causing a couple of warnings

*hooks/install, line 59 gives - 'unbound variable for ENV'

Comments
*It would be ideal if the user could import their own SSL certificate (even have the choice of not using SSL config?). You've mentioned this on irc already. SSL certs could be transmitted as a base64 encoded string or delivered as part of the charm deployment. See the Apache charm (http://jujucharms.com/charms/precise/apache2) for an example of the latter. A self-signed certificate can be generated when the user doesn't specify one.

*It'd be better if configuration was applied in a 'config-changed' hook, allowing the user to change things after deployment, e.g. apply a renewed SSL certificate.

Please fix the bugs and consider the comments, then resubmit for a second review.

[José Antonio Rey (jose)]

Robert:

The entries 'postfix/bad_recipient_delimiter' and 'postfi/not_configured' are in there, but have the entry type 'error'. Any idea on how can I fix it? Also, I am fixing the other bugs.

About the comments, I am figuring out how to get the SSL certificate on the charm from a private location, and getting that on the config-changed hook so people can renew it, and getting instructions on the README file in case they do not know how to generate one.

Once I've got those things ready, I'll resubmit for revision.

Thanks!

[Launchpad Janitor (janitor)]

[Expired for Juju Charms Collection because there has been no activity for 60 days.]

[José Antonio Rey (jose)]

Hey guys, I'd like to present a resumbission of the charm. I've fixed all the bugs and considered all the comments listed by Robert, and I think it's ready.

[Marco Ceppi (marcoceppi)]

Reviewing now

[Marco Ceppi (marcoceppi)]

Hi Jose, Thanks for taking the time to work on this charm, we really appreciate the submission!

Going to jump right in to the `charm proof` output:

W: Metadata is missing categories.
W: No icon.svg file.
- We've added icon's and categories since you last submitted this charm. You can find information about this: https://juju.ubuntu.com/docs/authors-charm-policy.html#charm-metadata and https://docs.google.com/a/canonical.com/document/d/1hBTH_7RLUYMV-VhZOGxb-bFHICXyVRU4t8L79Dlea80/pub
E: no copyright file
- This is a requirement and hard blocker, your charm files must be licensed under a free license http://opensource.org/osd you can use. You can use any charm in the store as a guide for format, there are many different ways to include a copyright file
I: relation mta has no hooks
- This isn't so bad, if there is no data to exchange
W: install not executable
W: start not executable
W: stop not executable
- Make sure all hooks have +x permissions on them!

# Blockers

- Install hook isn't idempotent. You move files around in several places, the results are if the install hook is ever run again it will error out

# Minor

- There's no way to change configuration once you've installed the charm. I recommend implementing a hooks/config-changed hook so users can change a configuration option after deployment and have it take effect
- I'd consider making the four certs required for SSL a part of configuration. That way a user doesn't have to fork the charm just to add certificate support (also, might be nice to have a self-signed certificate generated)

Great progress on the charm so far! I look forward to the next review as it's starting to take shape. Feel free to ask any questions by replying to this bug, sending a mail to the <email address hidden>, or as always on #juju in freenode!

[José Antonio Rey (jose)]

Resubmission for review.

[Kapil Thangavelu (hazmat)]

I'd change the interface from 'postfix' to smtp. This is pretty standard smtp+tls setup. Because of the auth setup though it doesn't seem very useful for connecting other services to, its basically delegating auth to saslauthd which is doing a host login check. More interesting would be if it created non login users in response to relations, so that other services could be related and then be able to send outbound email.

[José Antonio Rey (jose)]

I actually wrote this one because I wanted it to connect to mailman. It can, though, receive email, so smtp would not be the only interface here, we have pop3 too. Hence, postfix as the interface name.

[Mark Mims (mark-mims)]

Hi Jose,

Thanks for the charm submission!

Biggest problem I've identified so far is that there's no way for the add-ssl and update-ssl hooks to be called by the user. Please integrate these into the config-changed hook so the user can specify to use embedded certs or generate new ones.

Thanks!

[José Antonio Rey (jose)]

Fixed that bug.

[Marco Ceppi (marcoceppi)]

Hi Jose, thank you for your fixes and patience during this review process!

The charm is definitely coming together, and is pretty close to store acceptance. The concern I have is the SSL portions. It'd be much more preferable to be able to add the SSL certs via the configuration option instead of manually having to copy them to the server. Something like

juju set postfix ssl-key=`cat ~/smtpd.key` ssl-cert=`cat ~/smtpd.crt`

That way this charm can also be managed from the GUI.

[José Antonio Rey (jose)]

Fixed it!

[Benjamin Saller (bcsaller)]

Hi,

Thanks for your continued work on this. I had some comments that follow:

metadata.yaml:
    Having an 'mta' interface with no hooks seems unwarranted to me. Is this
    only so that the public address is available?

config.yaml:
    Useless defaults should be ommited, those are properly elements for the description
    but cannot be used to properly start the service and thus are not 'defaults'

    Can't config-get return an empty string for missing values? This seems better than
    'false' when tested with [ -n "$SSL" ]

config-changed:
    SSL test says =! which I believe is an error, however I think
    [ -n "$SSL" ] is a valid test as well if the defaults are dropped.

add-ssl:
    Does a service restart. I think I'd rather see this at the bottom of config-changed
    as that will likely need the restart two and it should only occur in one place.

README:
    Mentions both add-ssl and update-ssl helpers. only add is present or needed.

    80 Column word-wrap. I don't know that we insist on this, but its concidered
    good practice. This also applies to the copyright file.

    The openssl keygen example in the readme is missing a new line, leading to
    muliple comamnds on a sinlge line and is confusing.

Thanks

[José Antonio Rey (jose)]

Fixed everything. Yes, the mta interface is there to have a public address available.

[Charles Butler (lazypower)]

Greetings José,

I'm doing a preliminary review of your charm, and I'm really excited to see the amount of work that's gone into this charm. I want to thank you for your continued perseverance in releasing a quality charm. We really do appreciate the volume of effort placed into ensuring the charm is of a high quality.

The following issues cropped up when looking through the charm source:

- config.yaml
It appears that when you removed the defaults you removed the type specifiers in config.yaml. This prevents the charm from being deployed. I've issued a merge request against your branch to re-add these specifications for charm deployment.

- install
When running through the installation script I received the following error: hooks/install: line 60: PWDIR: unbound variable
It appears you missed setting the PWDIR variable in your script, instead of expanding as expected inline its looking for an unbound reference to PWDIR

*footnote* - i also noticed the output of the config was a bit wonky after editing the config-changed script to run until completed:
-- snip --
OPTIONS="-c -m /var/run/saslauthd"
START=yes
PWDIR=/var/spool/postfix/var/run/saslauthd
-- snip --

You may want to validate this against your intended output, or even move the file into a template, files, or contrib directory in the charm and generate from that. When re-running the script multiple times under failure conditions the installation script happily appends multiple duplicate lines to /etc/default/sasauthd

- config-changed
Ben made reference to your conditional operator returning an error: hooks/config-changed: line 15: conditional binary operator expected. His correction was to change this line to [ -n "$SSL" ]

Great progress on the charm so far! I look forward to the next review as it's starting to take shape. Feel free to ask any questions by replying to this bug, sending a mail to the <email address hidden>, or as always on #juju in freenode! I'm moving the status of this bug to incomplete barring the requested changes. Once you have made the changes, feel free to move the bug stats to 'new' or 'needs review'

Thanks again for your contribution José!

[José Antonio Rey (jose)]

All fixed for now!

[Charles Butler (lazypower)]

Jose,

In addendum to our talk on IRC it appears that the inline bugs have been corrected. The charm proof now returns a single informative message about no hooks on the MTA interface. Another charmer may have additional notes about this, but I don't see the harm as an initial revision.

This charm has passed my initial +1 review, and is now placed back in the charm rev queue for another charmer to perform the full and complete evaluation for the charm store.

Thank you for your hard work and dedication to making this charm the best it can possibly be.

[Matt Bruzek (mbruzek)]

Hello Jose,

Thanks for your continued improvements on this charm. I am also giving this charm a review and I found a few minor things that you should consider fixing.

When I run: juju charm proof postfix I see the informational message:
I: relation mta has no hooks

I see this was mentioned in previous reviews and I am OK with that informational message.

The README is very well written with an example for how to deploy the postfix charm. While following the instructions I found an error that should be fixed to avoid problems for others. I needed to add quotes around all the cat commands for the keys.

juju set postfix ssl-key="`cat ~/smtpd.key`" ssl-cert="`cat ~/smtpd.crt`"
  cacert="`cat ~/cacert.pem`" cakey="`~/cakey.pem`"

The set command would not work with out the double quote ("). Please add that to the Generating key section and Renewing keys section of the README.

Thanks for your persistence and patience on this charm review. I think we have a really nice one here and I am adding my +1 to this review. I believe this charm is ready for the charm store, but I am not in the charmer group I can not promote. We will have a charmer review this soon.

[José Antonio Rey (jose)]

Hey, Mark!

Just fixed those on the branch. If you guys find anything else to fix let me know, I'll be happy to. Will just wait for the inclusion on the store.

[Marco Ceppi (marcoceppi)]

Promulgated