Code review comment for lp:~adam-collard/charms/precise/landscape-client/upgrade-charm-hook
Revision history for this message
| Adam Collard (adam-collard) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
67d34a1
(Get the code!)
> [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(
>
> 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_
> + sys.exit(exit_code)
>
> Please add a docstring.
Done.
> [4]
>
> + meta_data_dir = os.path.
> + meta_data_dir = getattr(
>
> I find this code hard to follow. A slightly more verbose version would
> be easier, I think:
>
> if client_
> meta_data_dir = client_
> else:
> meta_data_dir = os.path.
>
> Or simply renaming the variable:
>
> fallback_
> data.d")
> meta_data_dir = client_
>
> 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(
>
> fromkeys is a classmethod, so let's use the class instead
> of an instance:
>
> juju_info = dict.fromkeys(
>
> 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-
> + "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_
> 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://
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.