New charm: mailman

Bug #1199052 reported by José Antonio Rey
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Juju Charms Collection
Undecided
Unassigned

Bug Description

Revision history for this message
Mark Mims (mark-mims) wrote :

Hi Jose,

This looks great!

Here're a couple of things in the first review pass...

- this looks like it's using it's own installed transport agent (postfix)... instead of using a relation with another transport agent. We should implement the relation logic to use an external mta... then add the option for the user to use a local mta or to _relate_ to an external mta via hooks like mta-relation-joined and mta-relation-changed.

- This service should really implement a 'website' relation that implements the 'http' interface. That way it could be related to a web proxy and/or other web-consuming services.

- charm proof gripes about some things:
{{{
hawk:~/reviews $ charm proof mailman
E: Unknown root metadata field (category)
W: Metadata is missing categories.
I: relation mailinglist has no hooks
I: relation mta has no hooks
}}}

- not sure what a 'mailinglist' interface would look like? What would go in the hooks for this kind of relation? What sort of services would consume that? drupal, wordpress, or something like that?

Awesome progress, let's see which direction you would like to go.

Changed in charms:
status: New → Incomplete
Revision history for this message
Launchpad Janitor (janitor) wrote :

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

Changed in charms:
status: Incomplete → Expired
José Antonio Rey (jose)
description: updated
Changed in charms:
status: Expired → New
Revision history for this message
Charles Butler (lazypower) wrote :

Greetings Jose,

Thank you for your work on this charm. I have been looking forward to this release since the postfix charm was submitted for review. I have a few notes and concerns that have been outlined below.

the charm passes charm proof, excellent!

in metadata.yaml - you define mailinglist as interface html. Why not use website or http so you can hook mailman into load balancers and reverse proxys?

During the config-changed hook execution, it fails the deployment with the following output:
PID unreadable in: /var/run/mailman/mailman.pid
[Errno 2] No such file or directory: '/var/run/mailman/mailman.pid'
Is qrunner even running?
Site list is missing: mailman
   ...fail!

barring this fix, I didn't see anything else inherently out there with the charm. You are very close to getting the mailman charm into the charm store. Thanks again for the work you've invested.

I'm going to place the status of this bug as "incomplete". When you are ready for another review simply move the status to "new" or "Fix committed" and someone will be along shortly to review your work.

Changed in charms:
status: New → Incomplete
José Antonio Rey (jose)
Changed in charms:
status: Incomplete → New
Revision history for this message
Matt Bruzek (mbruzek) wrote :

Hi Jose,

Thanks for your patience on the review process. I took a look and here is what I found:

Your README.md does not render properly in Markdown, just a few things to fix:

The steps to generate the keys and certificates:

The line starting with “3650” breaks the block quote format. There is no limitation on the file being less than 80 characters (especially within block quotes) so just make that command one long line.

Also the lines starting with “cacert=” do not render in the juju set block quotes as I believe they should. Same advice here, make that line one long line.

The Contact Information page does not render correctly for me either. If you want to represent a line break, end the line with 2 spaces.
Author: José Antonio Rey <email address hidden>(space)(space)
Report bugs at: http://bugs.launchpad.net/charms(space)(space)
Location: http://jujucharms.com
(space)(space)

Revision history for this message
José Antonio Rey (jose) wrote :

It's all fixed now if you want to take a look! Thanks!

Revision history for this message
Matt Bruzek (mbruzek) wrote :

José,

Thanks for your continued patience with the review process. You have been very responsive on IRC and I really appreciate that. I grabbed this charm off the review queue today and believe you are very close, but I still have a few recommendations.

The README contains references to “edit your config.yaml” in 2 different places. Editing the config.yaml is not the normal use case. Some customers may not even know about the config.yaml file if they deployed mailman charm in the juju-gui. I would recommend changing the README to suggest creating a new YAML file (mailman.yaml) with all the configuration that is required and then give the users the command to deploy with a configuration file: juju deploy mailmain –config=mailman.yaml
Or instruct the users to set the configuration values can be set by: juju set mailman key1=”value” key2=”value” something generic like that.

According to the known limitations section the charm will not be ready until the password is set. I would recommend listing the password field more prominently. Either list password near the top of the configuration section, or put quotes around `password` like the other config values (I didn't notice it when I read the README the first time.

After chatting on IRC you gave me a handy administration url. You might want to update the README with a reference to: http://yourdoma.in/cgi-bin/mailman/admin/mailman as well.

This charm deployed to HP cloud without any hook errors! I did hit a problem though and thought it might be useful to see exactly how I deployed mailman:

mkdir precise
bzr branch lp:~jose/charms/precise/mailman/trunk precise/mailman
juju deploy --repository . local:precise/mailman
juju set mailman password="thisisatest"

After setting password I was not able to create a new list using the following url: http://15.185.88.13/cgi-bin/mailman/create

I always got: Error: You are not authorized to create new mailing lists

I also tried this on my local provider and was not able to create a list there either. Let me know if I am doing something wrong here, but I followed the steps in the README and was unable to create a list.

I am going to put this bug back in Incomplete, when you fixed the create list problem please put it back in New or Fix Committed and it will be back in the review queue.

Changed in charms:
status: New → Incomplete
Revision history for this message
José Antonio Rey (jose) wrote :

Hey!

Everything has been fixed. I've also done a little clean-up on the code and the README has been updated accordingly.

Thanks!

Changed in charms:
status: Incomplete → Fix Committed
Revision history for this message
Charles Butler (lazypower) wrote :

Hey Jose,

Thanks for the submission and dedication to the mailman, I've done a charm review and everything looks great this time around. Thanks so much for the dedication and patience during the review process.

You can start monitoring for bugs : https://bugs.launchpad.net/charms/precise/+source/mailman

Congratulations on making it into the charm store! I look forward to the review once the trusty charm is completed.

Thanks again!

Changed in charms:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers