Code review comment for lp:~nick-moffitt/charms/precise/keystone/migrate-sqlite-to-mysql

Adam Gandelman (gandelman-a) wrote :

Nick-

Yes, you're right about the config options being set in the install hook being a bug. They were originally there to work on old deficiencies in packaging that are fixed now, and could probably be dropped from there / moved to config_changed.

That said, I'm not sure about duplicating the apt-get commands wholesale in upgrade-charm. As its proposed here, if there has been a newer package published to the archive for Keystone (or any other package that may have charm modified configs) since the last charm upgrade, apt-get will block waiting for user guidance on how to handle the config changes (merge, overwrite, shell, etc) and the upgrade hook will never complete. I see a couple of options:

- Call apt-get /w "--option Dpkg::Options::=--force-confnew -n" to ensure we keep our juju modified configs in place. This could be problematic, though, if the updated packages contain critical config file changes, too.

- Avoid calling apt-get in the upgrade hook and instead late install the packages required for migration as needed in migrate_db(), similar to how subprocess is being imported late there.

The latter seems safest to me, since we're only acting on the migration related packages, and ideally only gets run once upon relating to a new mysql server. What do you think?

review: Needs Fixing

« Back to merge proposal