Charm needed: znc

Bug #956259 reported by Jorge Castro
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Juju Charms Collection
Fix Released
Undecided
Patrick Hetu

Bug Description

People need IRC bouncers!

Tags: new-charm

Related branches

Revision history for this message
Jorge Castro (jorge) wrote :

Patrick I've filed this as a placeholder to keep track of znc, when you'd like a review tag it with new-charm. Thanks!

Changed in charms:
assignee: nobody → Patrick Hetu (patrick-hetu)
tags: added: new-charm
Revision history for this message
James Page (james-page) wrote :

Hi Patrick

Thanks for submitting this charm for review.

Some feedback:

1) charm configuration:

There is alot of znc specific syntax/configuration in the config.yaml for this charm. I think it would be better to try to abstract the users of the charm away from the implementation details.

With this in mind its probably better to go with a single port configuration rather than one for each - I noticed that you strip out everything side from the number anyway in the install hook.

Makes the open-port command easier as well.

With regards to the use of XML in the config.yaml - I don't think this is a good idea.

Again I think this should be abstracted into a minimal set of data which the install hook uses to generate the configuration for znc; maybe username, nick, password, network and a list of channels. This is probably enough to get people bootstrapped.

They can then use the web interface to add additional configuration later.

2) start/stop hooks

As you install upstart configuration for znc these are not really required IMHO.

3) install

I had problems with:

su -c "/usr/bin/znc -d /var/lib/znc --makepem" znc

Running the local provider - this command was prompting for input - might be better to try to pass some sensible defaults to this as a fully qualified domain name is not always configured (I think this is the issue).

Generally it would be good to make this hook a bit more resilient to being executed multiple times.

Please address this feedback and mark this bug as 'Fix Committed' when its ready for review again.

Changed in charms:
status: New → Incomplete
Revision history for this message
James Page (james-page) wrote :

Patrick

I also had problems with the "ProtectWebSessions = true" znc configuration element - znc did not seem to like this in oneiric.

Revision history for this message
Patrick Hetu (patrick-hetu) wrote : Re: [Bug 956259] Re: Charm needed: znc

I followed all your recommendations and I have one question:

> Generally it would be good to make this hook a bit more resilient to
> being executed multiple times.

The install hook should only be run once no?

Changed in charms:
status: Incomplete → Fix Committed
Marc Cluet (lynxman)
Changed in charms:
assignee: Patrick Hetu (patrick-hetu) → Marc Cluet (lynxman)
status: Fix Committed → Confirmed
status: Confirmed → Incomplete
Revision history for this message
Marc Cluet (lynxman) wrote :

Hi Patrick,

After reviewing your charm here's a couple more suggestions

In install hook:
znc shouldn't be started from this hook unless you have a very good reason to do so, the install hook needs to be idempotent (as all the other hooks) so make sure that the install hook can be executed as many times as possible without changing the outcoming result.

In start hook:
You should add a start hook that starts znc in a reliable way, checking that another instance doesn't exist before starting a new one and so forth, when you deploy the charm for the first time juju will run the install and then start hook in order

In stop hook:
Same as the start hook, make sure that you kill the process that you started, make sure to clean afterwards if you need to remove pids or other temporary files

In config-change hook:
You should make sure a config-change hook exists so people can change your charm configuration through "juju set", keep in mind that with juju the user shouldn't have to log into the machine to modify the application configuration

Revision history for this message
Patrick Hetu (patrick-hetu) wrote :

I've followed your suggestions: I added the start/stop hooks and moved the configuration code from the install hook to a config-changed hook.

Changed in charms:
status: Incomplete → Fix Committed
Marc Cluet (lynxman)
Changed in charms:
status: Fix Committed → In Progress
Revision history for this message
Marc Cluet (lynxman) wrote :

Looks perfect! Thanks Patrick :)

Changed in charms:
status: In Progress → Fix Committed
Revision history for this message
Marc Cluet (lynxman) wrote :
Changed in charms:
status: Fix Committed → Fix Released
Changed in charms:
assignee: Marc Cluet (lynxman) → Patrick Hetu (patrick-hetu)
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.