Code review comment for lp:~adam-collard/charms/precise/landscape-client/upgrade-charm-hook

Revision history for this message
Adam Collard (adam-collard) wrote :

 > [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.


> [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": "",
> + "private-address": ""}, 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 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.

« Back to merge proposal