Comment 1 for bug 1414204

Anthony,

Thank you for this charm submission. This looks like it will be very useful in helping keep units patched and up to date. We had a few comments, however, only one of which I really consider blocking.

First, we very much appreciate that you included a test in this charm. However, the fact that it requires manual input means that it will cause issues with our automated test runner, bundletester. We checked to confirm that the charm deploys successfully when given a bogus accesskey, and, while obviously the ksplice / uptrack service will not actually work, it should be enough for the 01_deploytest.py to pass. Barring getting a "test key" from Oracle that would allow us to actually test the functionality of the deployed KSplice, I think confirming the deployment with a bogus key is going to be the best test we can have.

Second, while the charm is generally very cleanly implemented, and you are including charmhelpers and making some use of it, you are also reimplementing several functions that are available in charmhelpers. I'd like to recommend that you consider refactoring the charm to use the following alternatives:

  * utils.config_get -> core.hookenv.config
  * utils.juju_log -> core.hookenv.log
  * utils.config_repo -> fetch.add_source
  * utils.render_template -> core.templating.render
  * resources.directory -> core.host.mkdir

I also noted that `charm proof` gives informational messages due to the start and stop hooks being missing. You could put no-op placeholders in for these hooks to clean up these messages, but this is definitely non-blocking.

Finally, you have the charm going into an (error) state if the accesskey config option is not set, thus requiring the user to `juju resolved --retry` the charm after setting the option. It might be nicer to simply log the CRITICAL message and defer the setting of the package config, so that it is just a one-step process to resolve, but I can understand the argument that there is not yet a better way to communicate to the user that manual intervention is required.

Thanks again for this charm, and I look forward to getting it approved into the store.