Merge lp:~evarlast/charms/precise/mysql/encoding into lp:charms/mysql

[Jay R. Wren]

  use encoding from relation for create db

  https://bugs.launchpad.net/charms/+source/mysql/+bug/965094

[Chris Glass (tribaal)]

This seems fine, if the relation does not set an explicit encoding, it defaults to utf8 (which is sensible).

The problem in my view is that it is not part of the formal interface definition of the "mysql" relation, as described here: https://juju.ubuntu.com/docs/interface-mysql.html

I would be ok to merge this branch (if I could), if I was shown a branch against the documentation updating the formal interface definition. It matters because it seems the mysql interface is our "golden standard" interface, and pretty much the only formalized interface in the whole ecosystem.

[Marco Ceppi (marcoceppi)]

I wouldn't say the MySQL interface is formalized, just the first one "documented". Interfaces are designed (and loosely typed) so they can evolve overtime, much as this is a welcome and shown improvement. Chris, I think you and I are on the same page and appreciate the caution in merging.

Updating the documentation would be a huge plus for this. Jay, you can find the documentation for juju on Github in Markdown. Including the optional encoding key would indeed speed these merges along: https://github.com/juju/docs

Otherwise, +1 from me

[Jay R. Wren (evarlast)]

Sounds good. Here is a PR for updating that documentation.
https://github.com/juju/docs/pull/137

On Tue, Aug 5, 2014 at 7:26 AM, Marco Ceppi <email address hidden> wrote:

> I wouldn't say the MySQL interface is formalized, just the first one
> "documented". Interfaces are designed (and loosely typed) so they can
> evolve overtime, much as this is a welcome and shown improvement. Chris, I
> think you and I are on the same page and appreciate the caution in merging.
>
> Updating the documentation would be a huge plus for this. Jay, you can
> find the documentation for juju on Github in Markdown. Including the
> optional encoding key would indeed speed these merges along:
> https://github.com/juju/docs
>
> Otherwise, +1 from me
> --
>
> https://code.launchpad.net/~evarlast/charms/precise/mysql/encoding/+merge/226538
> You are the owner of lp:~evarlast/charms/precise/mysql/encoding.
>

[Cory Johns (johnsca)]

Jay,

Thanks for getting those docs updated. However, I see an issue with this change. The relation-joined hook will execute before the other side has any chance to call relation-set to set the encoding. At that point the database will have been created with the default encoding, and the relation-set will have no effect. This was borne out in my tests; I could not get the encoding to take effect.

This logic needs to be moved into a db-relation-changed hook, which means you will need to either delay creating the database, or change it to use ALTER DATABASE to update the encoding. (The latter option also raises the question of whether it should be possible to update the encoding after the database has been in use and has data, but the documentation only describes using it in the relation-joined hook, so maybe that is a non-issue.)

Again, thanks for this contribution. It will be an excellent addition once this issue is resolved.

review: Needs Fixing

[Jorge Castro (jorge)]

Thanks for the contribution Jay, I'm moving this to need's fixing, please change it back to "Needs review" when you're ready for round two!

(Also mail me your address, I have a juju shirt for you).

[Jorge Castro (jorge)]

review: Needs Fixing

Unmerged revisions

125. By Jay R. Wren on 2014-07-11

use encoding from relation for create db

https://bugs.launchpad.net/charms/+source/mysql/+bug/965094

=== modified file 'hooks/db-relation-joined'
--- hooks/db-relation-joined	2012-11-02 06:41:12 +0000
+++ hooks/db-relation-joined	2014-07-11 21:10:23 +0000
@@ -1,6 +1,7 @@
 #!/usr/bin/env python
 
 from common import *
+from charmhelpers.core import hookenv
 
 import urllib
 import subprocess
@@ -68,7 +69,9 @@
       print "database exists already"
   else:
       if not broken and not admin:
-          runsql("create database `%s` character set utf8" % database_name)
+          encoding = hookenv.relation_get().get('encoding', 'utf8')
+          runsql("create database `%s` character set %s" % (database_name,
+              encoding))
           with open(database_name_file, 'w') as dbname:
               dbname.write(database_name)