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

Revision history for this message
Björn Tillenius (bjornt) wrote :

Looks good in general. Even though we said this won't be needed for IS, I
still think it makes sense to get this landed, since there might be other
deployments out there that we don't know about. I do have some questions
and nitpicks, though :)

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

[2]

-chown(juju_info_file)

Why was this removed? Why isn't it necessary to change the owner of the
file anymore?

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

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

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

review: Needs Information

« Back to merge proposal