Merge lp:~marcoceppi/charms/oneiric/minecraft/trunk into lp:charms/oneiric/minecraft

[Marco Ceppi]

Updated to nix old form of checksum checking (since HTTPS) and added upgrade hook

[Clint Byrum (clint-fewbar)]

Hi Marco, the download changes look good. +1

The name of 'upgrade' should be 'upgrade-charm' if you mean for it to be runnable by juju.

IMO, upgrade-charm should just call stop, install, config-changed, and start, rather than have a special method just for checking for upgrades, since you might have changed any of those hooks as well.

Also, minor niggle.. 'cmp' is better for comparing binaries than diff.

review: Needs Fixing

[lp:~marcoceppi/charms/oneiric/minecraft/trunk]

27. By Marco Ceppi on 2012-03-13

Proper name for upgrade hook

28. By Marco Ceppi on 2012-03-13

Look! The stop hook does something

29. By Marco Ceppi on 2012-03-13

Updated hooks to be better players at the idempotent game

[Marco Ceppi (marcoceppi)]

Hey Clint,

Thanks for the review! I've changed the hook name to follow the proper naming scheme.

Also, per your recommendations I've set the upgrade-charm script to invoke stop, install, start hooks and moved the various logics for each around so they work in tandem with one another and also separately. Just as well I've replaced diff with CMP.

Thanks!

[lp:~marcoceppi/charms/oneiric/minecraft/trunk]

30. By Marco Ceppi on 2012-03-13

Delete the tmp file if we don't use it

[Clint Byrum (clint-fewbar)]

Looking quite good, seems a bit more elegant now.

One thing.. stop and start should also be idempotent if possible.. so

if status minecraft | grep -q stop/ ; then
  start minecraft
fi

This just makes things more resilient in case of unexpected system state.. as this asserts start but retains any error checking.

(and yes, we should probably push the logic above all the way back into upstart's 'start' utility so we cane make it something more like 'start --ok-no-change minecraft')

Thats really a nit pick though, the rest looks good. Also another nit.. there's not much good reason to source the scripts instead of run them... and sourcing them means variables used in both scripts might have unexpected values.. so I'd just drop the '. hooks/install' and make it 'hooks/install'. Again, just a nit! Approved.

review: Approve

=== modified file 'hooks/install'
--- hooks/install	2011-11-16 20:50:43 +0000
+++ hooks/install	2012-03-13 03:35:22 +0000
@@ -1,23 +1,28 @@
 #!/bin/sh
 
-## https://gist.github.com/1336585
-
-juju-log "Installing OpenJDK"
-apt-get install -y openjdk-6-jre
-
-juju-log "Add Minecraft init script" 
-cp opt/minecraft /etc/init/minecraft.conf
-
-juju-log "Downloading Minecraft"
-mkdir -p /opt/minecraft/
-wget "https://s3.amazonaws.com/MinecraftDownload/launcher/minecraft_server.jar" -O /opt/minecraft/minecraft_server.jar
-
-juju-log "Branching Minecraft Checksum"
-bzr branch lp:~marcoceppi/charm/oneiric/minecraft/checksum
-
-md5sum -c  checksum/checksums
-if [ "$?" -ne 0 ]; then
-	juju-log "Checksum conditions failed! Exiting"
-	exit 2 #Checksum error number, I just made it up for this.
+apt-get update && apt-get upgrade -y
+
+juju-log "Check if we've installed Minecraft before"
+
+if [ ! -f "/opt/minecraft/minecraft_server.jar" ]; then
+	juju-log "Installing OpenJDK"
+	apt-get install -y openjdk-6-jre charm-helper-sh
+
+	juju-log "Add Minecraft init script"
+	cp opt/minecraft /etc/init/minecraft.conf
+
+	juju-log "Downloading Minecraft"
+	mkdir -p /opt/minecraft/
+	wget "https://s3.amazonaws.com/MinecraftDownload/launcher/minecraft_server.jar" -O /opt/minecraft/minecraft_server.jar
+else
+	juju-log "Check for upgrades to current installation"
+	wget "https://s3.amazonaws.com/MinecraftDownload/launcher/minecraft_server.jar" -O /tmp/minecraft_server.jar
+	cmp /opt/minecraft/minecraft_server.jar /tmp/minecraft_server.jar > /dev/null
+
+	if [ "$?" -ne 0 ]; then
+		rm -f /opt/minecraft/minecraft_server.jar
+		mv /tmp/minecraft_server.jar /opt/minecraft/
+	else
+		rm -f /tmp/minecraft_server.jar
+	fi
 fi
-

=== modified file 'hooks/start'
--- hooks/start	2011-11-16 20:50:43 +0000
+++ hooks/start	2012-03-13 03:35:22 +0000
@@ -1,3 +1,4 @@
 #!/bin/bash
 
+juju-log "Starting Minecraft"
 start minecraft

=== modified file 'hooks/stop'
--- hooks/stop	2011-11-15 14:33:40 +0000
+++ hooks/stop	2012-03-13 03:35:22 +0000
@@ -1,1 +1,4 @@
-#!/bin/bash
+#!/bin/sh
+
+juju-log "Stopping Minecraft"
+stop minecraft

=== added file 'hooks/upgrade-charm'
--- hooks/upgrade-charm	1970-01-01 00:00:00 +0000
+++ hooks/upgrade-charm	2012-03-13 03:35:22 +0000
@@ -0,0 +1,7 @@
+#!/bin/sh
+
+juju-log 'Running an upgrade'
+
+. hooks/stop
+. hooks/install
+. hooks/start