Juju Charms Collection

Bug #1125869
Comment #16

Comment 16 for bug 1125869

Revision history for this message

Charles Butler (lazypower) wrote on 2014-01-20:

#16

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é!

Greetings José,

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 --

- 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 juju@lists.ubuntu.com, 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é!