charm for freeciv-server

Hi Jerry,

Great job on getting this charm up! I went through and reviewed was was done so far and have a few items of feedback for you :) First off, this is an awesome charm, I know I'll be using it frequently!

1) There doesn't appear to be any way to configure the Freeciv server except for the defaults in the freeciv-server package. Consider setting up a config-changed hook and config.yaml[1] file to allow users to update settings[2] for the server. You have a bunch of different options in regards to this:
a) Explicitly list all the logical configuration options in a config.yaml file so users can juju set <option=val> which would then execute them in the running screen instance
b) Create a config option for a "command-line user" so an in-game user can be specified as an admin
c) A mixture of the two, some of the vital options abstracted to command-line and the ability to add cli user which can update the rest in game.

In order for this to be a rock solid charm though, some form of configuration would be required, IMO.

2) You don't need to killall freeciv-server from the start hook since start hook is only run once by juju.

/** None vital, but something to consider **/
3) You might want to create an upstart script for freeciv-server, though given the nature of the charm it would probably be more trouble than pay-off since the service never needs to be restarted for configuration updates, like Apache and other services.

 So far a great start to a sweet charm!

[1]: https://juju.ubuntu.com/docs/drafts/service-config.html#creating-charms
[2]: http://freeciv.wikia.com/wiki/Server_options

Hi Jerry, I saw some commits on your branch, is this ready for resubmission? Thanks!

Hi Marco,

I think I have addressed the issues you outlined. Issue #3 (the upstart script) is not applicable since freeciv server runs as a terminal application and not a daemon. The user of the charm will likely connect to the server via ssh and begin typing commands on the freeciv server console for things like map selection. I don't know freeciv well so I could be putting myself up for correction. :)

The command line options can now be configured and reconfigured using juju. Some things like the location of the save files I did not expose because I figure if someone is running a dedicated machine for freeciv, they will probably not care to change the default setting.

Thanks for the timely and thorough review!

Reviewing this now.

-Juan

I found the branch at: lp:~jseutter/charms/oneiric/freeciv-server/trunk

Hi:

Some feedback on the charm. charm proof freeciv outputs the following:

W: no README file
I: relation freeciv-service has no hooks

Charms need to have a README file. It's nice when we have a README file that explains what the charm does and how it should be used. Also an explanation of the different options that could be configured in it.

One of the great advantages of juju is it's ability to connect services to one another based on relationships. Hooks are the way we initiate the relationships on each charm. Each charm should provide ( or maybe require ) something that has a hook associated with it.

This may or may not apply to _every_ charm ( hence the information and not warning or fatal ).

In this case, the metadata.yaml file specifies a freeciv-service relation that has no hook.

All in all the charm, deploys, works and it's very interesting.

If you can fix the issues above and resubmit, I would be happy to review again.

-Juan

Hi Juan,

I added a README file. Please read it and let me know if it sounds reasonable to someone other than myself.

As far as relations hooks, there is really nothing applicable for the Freeciv server. The service is self-contained and only Freeciv clients use it. The only thing I could think of is providing some sort of IP address -> domain name hook... Thoughts?

Thanks,

Jerry

Hi Jerry:

Thanks for taking the time to continue to update the charm.

== Blockers ==
- We recently added a requirement for charms to have a maintainer field in the metadata.yaml
  It would look something like:
  maintainer: Jerry Seutter <email address hidden>
  As a rule of thumb, it's a good idea to run charm proof ( charm proof <charm> ) before submitting.
  It tries to keep up with changes to the rules as well as new things we find need to be tested.

== Non Blockers ==
- One thing you can do with the freeciv-service hook would be to create a simple join hook that adds it's IP to the realtionship ( relation-set host=`unit-get private-address` ) just to satisfy the hook. I understand that this is a self contained charm but, in the future, we can come up with ways of using this charm ( I think ) that may take advantage of the hos/ip being exported in a relation . That's one of the cool things about Juju and charms, we expose information about a service and some other unrelated charm can just use it.

Just my two cents ...

Adding the maintainer field to the metadata.yaml file would take care of the blockers and I can review/approve when ready.

Thanks,

Juan