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

Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

Looks good, but I think we should address [1] and [2].

[1]

+def upgrade_charm():

Since we might need to do other things in this hook in the future, it'd be better to keep it lean and extract the logic into standalone (testable) functions. E.g.:

def upgrade_charm():
    client_config = get_client_config()
    migrate_old_juju_info(client_config) # passing in client_config should make this function fully testable
    # future upgrade steps will go here

[2]

It'd be good to test this logic.

review: Needs Fixing

« Back to merge proposal