Charm needed: minetest server

Hi James

Nice work so far!

A few comments:

1) Hooks with no hooks/*

Your charm declares a provides hook:

provides:
    server:
        interface: minetest

But does not implement any hooks to support this; I suspect its just not required as clients connect directly (not through another Juju server).

2) hooks/install

Needs a bit more idempotency:

juju-log "Adding minetest user and group"
addgroup --system minetest
adduser --system --home /srv/minetest --ingroup minetest --gecos "Minetest server" minetest

This segment will fail if run twice -and it might be as the upgrade-charm hook runs the install hook. I'd nest each call in a conditional that check to see if the group/user exists first.

3) hooks/config-changed

I don't think that you will be able to change configuration post deployment with sed's like this:

    sed -i -e "s/^#port \= .*/port \= ${PORT}/" "$CONFIG_FILE"

Once the configuration option has been set, its not going to be picked up by the pattern match on subsequent executions.

I'd actually recommend a different approach; how about templating the configuration file in the charm; this gives you a better chance of repeatability when it comes to configuring minetest.

Cheers

James

Hi James,

thanks for the pointers. Unfortunately I missed the charm school last week due to a scheduling clash, but hopefully I'll be able to put an hour aside to watch it and get a little more familiar with Juju.

You're right about the clients "connecting" directly (the transport is UDP, so it's not a connection as such). I'm only aware of one other server-side component that might want to talk to that interface, and that's the "master" server. It's not currently packaged, but if time permits I hope to change that. Maybe the server interface isn't required at this point, or even at all - I need to better understand the relationship between the provides: section in the metadata and the hooks directory and other services.

I've committed and pushed a fix for the user and group creation, thanks for pointing that out.

I'll take a look at the config-changed hook as soon as I get the chance. Templating did figure in my thinking, but since the example configuration file is included in the package I was conscious of introducing another version of that file that could get out of sync.

On an unrelated note, the version of minetest in the archive is actually quite old - I think the latest stable is now 0.4.6 and the project is under active development. I was thinking about adding another configuration flag to select which version of the package to install - the archive version, or one from the stable or daily PPA. Any thoughts/tips/warnings in that regard?

Cheers,

James

Hi James,

Because this charm was marked as incomplete we never saw this charm as being ready for review. Since this has been a while it's probably bitrotted. How would you like us to proceed?

Thanks, and sorry that this slipped through the cracks.