> [1] > > +lint: > + bzr ls-lint > > I'm not sure this is needed. writng "bzr ls-lint" isn't much harder then > "make lint", and the latter is confusing for non-landscapers, since > ls-lint is a landscape-specific plugin. lint is a make target for server, client and server charm. I think adding it to the client charm completes the symmetry. In addition, given that the client already has this target and is also open for non Landscapers I don't think it's any more confusing. Without that I'd need to document what linting rules should be used - the Makefile serves that purpose and additionally is executable :) > [2] > > -chown(juju_info_file) > > Why was this removed? Why isn't it necessary to change the owner of the > file anymore? It was removed to make passing the tests feasible without running as root. As I looked in to how I might restructure the code and/or tests to mock out the chown I realised that it simply wasn't necessary - as far as I can tell it was copy-pasta from the config re-writing piece that the charm also does. I verified that landscape user can read the file, and that's all they need to be able to do. > [3] > > +def upgrade_charm(): > + client_config = get_client_config() > + exit_code = migrate_old_juju_info(client_config) > + sys.exit(exit_code) > > Please add a docstring. Done. > [4] > > + meta_data_dir = os.path.join(client_config.data_path, "meta-data.d") > + meta_data_dir = getattr(client_config, "meta_data_path", meta_data_dir) > > I find this code hard to follow. A slightly more verbose version would > be easier, I think: > > if client_config.get("meta_data_path"): > meta_data_dir = client_config.get("meta_data_path") > else: > meta_data_dir = os.path.join(client_config.data_path, "meta-data.d") > > Or simply renaming the variable: > > fallback_meta_data_dir = os.path.join(client_config.data_path, "meta- > data.d") > meta_data_dir = client_config.get("meta_data_path", fallback_meta_data_dir) > > Notice that there's no need to use getattr(). I used this because it was exactly how the previous version of the charm did it. Will re-write it. > [5] > > + juju_info = {}.fromkeys(juju_key_map.values()) > > fromkeys is a classmethod, so let's use the class instead > of an instance: > > juju_info = dict.fromkeys(juju_key_map.values()) > > Although, you could also not create the dictionary with the > keys upfront, and instead compare the keys after you've migrated > the meta data files. Not sure which looks better, but it feels > more intuitive to check the presence of something, rather than > to see whether the value is None. I wanted juju_info to be strict about what keys are valid, and not just a general dictionary. To that end I wanted to create it up front and then populate the specific values. Error-checking then fell out as being looking for None. Have switched to using dict instead of {} though. > > > [6] > + self.assertEqual( > + {"environment-uuid": "0afcd28d-6263-43fb-8131-afa696a984f8", > + "unit-name": "postgresql/0", > + "api-addresses": "10.0.3.1:17070", > + "private-address": "10.0.3.205"}, juju_info) > > Great that you added some tests! > > I do think that tests should set up what they are testing, though. When > reading the tests, it's hard to see whether these values are correct. It > would be more clear if you had a helper method, create_old_metadata(), > which would accept a dict and write the files for you. If you have that, > calling that method in the test makes it easier to see what the code > is supposed to be doing. I shouldn't have to look at setUp() to understand > how a test works, IMHO. I had something a bit like that in http://bazaar.launchpad.net/~adam-collard/charms/precise/landscape-client/upgrade-charm-hook/view/30/hooks/test_hooks.py but then refactored into setUp. I did that because previous reviews left me with the feeling that helper methods aren't liked in tests. Would you prefer it if I revert to r30? Note that I don't ever need to have different contents so I like having that fixed in the helper itself.