Merge lp:~adam-collard/charms/precise/landscape-client/upgrade-charm-hook into lp:charms/landscape-client

Proposed by Adam Collard
Status: Merged
Approved by: Adam Collard
Approved revision: 36
Merged at revision: 28
Proposed branch: lp:~adam-collard/charms/precise/landscape-client/upgrade-charm-hook
Merge into: lp:charms/landscape-client
Diff against target: 197 lines (+139/-5)
5 files modified
Makefile (+5/-0)
hooks/container-relation-joined (+1/-2)
hooks/hooks.py (+47/-2)
hooks/test_hooks.py (+85/-0)
revision (+1/-1)
To merge this branch: bzr merge lp:~adam-collard/charms/precise/landscape-client/upgrade-charm-hook
Reviewer Review Type Date Requested Status
Björn Tillenius (community) Approve
Free Ekanayaka (community) Approve
Review via email: mp+189787@code.launchpad.net

Commit message

Add an upgrade-charm hook which will move any Juju information found in the old metadata.d directory into the new juju-info.json file

Description of the change

Add an upgrade-charm hook which will move any Juju information found in the old metadata.d directory into the new juju-info.json file (see https://code.launchpad.net/~ack/charms/precise/landscape-client/separate-juju-info/+merge/186592).

To post a comment you must log in.
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
30. By Adam Collard on 2013-10-09

Address free's feedback:
 * Add unit tests
   * Add Makefile with test and lint targets
   * Remove chown'ing of juju-info.json to facilitate testing
 * Extract migration code from generic upgrade-charm hook

31. By Adam Collard on 2013-10-09

Refactor into setUp

Revision history for this message
Adam Collard (adam-collard) 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

Done.

>
> [2]
>
> It'd be good to test this logic.

Added tests, and a Makefile to go along with them :)

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

Nice work, +1!

[3]

Would you mind creating a hooks/tests sub-directory and move test_hooks.py there? For consistency with client/server code, and to avoid cluttering the hooks dir (we should eventually have test_ceph.py, test_common.py etc).

[4]

+ class StubConfig(object):
+
+ data_path = mkdtemp(prefix=self.__class__.__name__)
+ meta_data_path = os.path.join(data_path, "metadata.d")
+
+ self.config = StubConfig()
+ self.addCleanup(rmtree, self.config.data_path)

What do you think of:

a) Using MockerTestCase, which sports the makeDir() method, saving
   boilerplate.

b) Using a real Configuration object, you just need to set the data_path
   attribute on it.

When possible it feels that using real objects is preferable, to eliminate the risk of interface mismatch with the stub.

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

> Nice work, +1!
>
> [3]
>
> Would you mind creating a hooks/tests sub-directory and move test_hooks.py
> there? For consistency with client/server code, and to avoid cluttering the
> hooks dir (we should eventually have test_ceph.py, test_common.py etc).

The reason it's alongside is that I wanted to match the server charm. I can move both of them if you think that's better?

>
> [4]
>
> + class StubConfig(object):
> +
> + data_path = mkdtemp(prefix=self.__class__.__name__)
> + meta_data_path = os.path.join(data_path, "metadata.d")
> +
> + self.config = StubConfig()
> + self.addCleanup(rmtree, self.config.data_path)
>
> What do you think of:
>
> a) Using MockerTestCase, which sports the makeDir() method, saving
> boilerplate.

I considered it but I didn't want to introduce any dependencies beyond stdlib. I don't think it's worthwhile given that we would then have to add a build step before being able to test the charm (which sits in isolation to the bigger pieces such as client/server)

>
> b) Using a real Configuration object, you just need to set the data_path
> attribute on it.
>
> When possible it feels that using real objects is preferable, to eliminate the
> risk of interface mismatch with the stub.

Makes sense. I didn't actually consider that one. Since we already have an import-time dependency on landscape-client being installed I don't have the same objection as a)

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

[3]

Yeah I'd do that too then, but you're call. It can be done separately.

[4]

Okay, I see the point. We're already depending on mocker on the server charm tho, and in general I think we shouldn't be wary of adding test dependencies when they make sense. Your call as well.

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
32. By Adam Collard on 2013-10-14

Add docstring to upgrade_charm.

33. By Adam Collard on 2013-10-14

More feedback from BjornT: make mea_data_path easier to grok, use dict not {} for fromkeys

34. By Adam Collard on 2013-10-14

Address Free's feedback - use real Configuration instead of a stub

Revision history for this message
Adam Collard (adam-collard) wrote :
Download full text (4.1 KiB)

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

Read more...

Revision history for this message
Björn Tillenius (bjornt) wrote :
Download full text (4.5 KiB)

On Mon, Oct 14, 2013 at 06:55:13AM -0000, 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 :)

Sure, fair enough.

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

Ok, sounds reasonable.

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

Well, you can still have it to be strict by checking to the keys after
it has been created. It's still a general dictionary, even if you create
the keys upfront. If you want it to be strict a class would be better. I
feel it's more naturally to check for the presence of keys for dicts,
but it's a matter of taste. I'm fine with checking for None as well.

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

Read more...

35. By Adam Collard on 2013-10-14

Separate creation of config from setting up of old juju info.

36. By Adam Collard on 2013-10-14

Avoid re-using the juju_info variable name

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

> > > [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.
>
> No, I don't think you should revert to r30, what you have there is
> basically exactly what you have now, except that you call setUp() in
> each test.
>
> Things that aren't directly important for the test should go in setUp,
> for example creating the config (although I agree with Free that you
> should use a real config object, not a stub one), and creating the
> config data path.
>
> What should be in the test is the juju_info creation. Since you check
> the keys and values in the assertion, you should also pass them in the
> method that writes the data. And you do have different data, you do an
> os.remove() call in at least one of the tests.
>
> Note that you could have default values for the tests that don't check
> the keys and values. But for those that check, it's easier to understand
> if you explicitly pass in the values.

Good spot on the os.remove() I'd forgotten about that. I've re-factored this so the tests are more explicit (don't need to grok setUp so much). Thanks for the suggestions!

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

On Mon, Oct 14, 2013 at 07:53:40AM -0000, Adam Collard wrote:
> > > > [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.
> >
> > No, I don't think you should revert to r30, what you have there is
> > basically exactly what you have now, except that you call setUp() in
> > each test.
> >
> > Things that aren't directly important for the test should go in setUp,
> > for example creating the config (although I agree with Free that you
> > should use a real config object, not a stub one), and creating the
> > config data path.
> >
> > What should be in the test is the juju_info creation. Since you check
> > the keys and values in the assertion, you should also pass them in the
> > method that writes the data. And you do have different data, you do an
> > os.remove() call in at least one of the tests.
> >
> > Note that you could have default values for the tests that don't check
> > the keys and values. But for those that check, it's easier to understand
> > if you explicitly pass in the values.
>
> Good spot on the os.remove() I'd forgotten about that. I've
> re-factored this so the tests are more explicit (don't need to grok
> setUp so much). Thanks for the suggestions!

Thanks, looks good now, +1!

  vote approve

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'Makefile'
2--- Makefile 1970-01-01 00:00:00 +0000
3+++ Makefile 2013-10-14 07:53:20 +0000
4@@ -0,0 +1,5 @@
5+test:
6+ cd hooks && trial test_hooks.py
7+
8+lint:
9+ bzr ls-lint
10
11=== modified file 'hooks/container-relation-joined'
12--- hooks/container-relation-joined 2013-09-20 07:19:18 +0000
13+++ hooks/container-relation-joined 2013-10-14 07:53:20 +0000
14@@ -4,7 +4,7 @@
15 import sys
16
17 from common import (
18- get_client_config, update_client_config, get_relation_config, chown,
19+ get_client_config, update_client_config, get_relation_config,
20 write_json_file)
21
22
23@@ -22,6 +22,5 @@
24 "private-address": relation_conf.get("private-address")}
25 juju_info_file = os.path.join(client_config.data_path, "juju-info.json")
26 write_json_file(juju_info_file, juju_info)
27-chown(juju_info_file)
28
29 sys.exit(exit_code)
30
31=== modified file 'hooks/hooks.py'
32--- hooks/hooks.py 2013-07-10 17:11:48 +0000
33+++ hooks/hooks.py 2013-10-14 07:53:20 +0000
34@@ -5,8 +5,8 @@
35 import base64
36
37 from common import (
38- update_client_config, clear_registration, get_service_config,
39- get_relation_config)
40+ update_client_config, clear_registration, get_client_config,
41+ get_service_config, get_relation_config, write_json_file)
42
43
44 def _write_certificate(certificate, filename):
45@@ -43,8 +43,53 @@
46 sys.exit(update_client_config(relation_data))
47
48
49+def upgrade_charm():
50+ """Idempotently upgrades state from older charms."""
51+ client_config = get_client_config()
52+ exit_code = migrate_old_juju_info(client_config)
53+ sys.exit(exit_code)
54+
55+
56+def migrate_old_juju_info(client_config):
57+ """
58+ Migrates data from the old meta-data.d directory into the new
59+ juju-info.json file.
60+ """
61+ fallback_meta_data_dir = os.path.join(
62+ client_config.data_path, "meta-data.d")
63+ meta_data_dir = client_config.get("meta_data_path", fallback_meta_data_dir)
64+ if not os.path.exists(meta_data_dir):
65+ return 0
66+ juju_info_file = os.path.join(client_config.data_path, "juju-info.json")
67+ juju_key_map = {"juju-env-uuid": "environment-uuid",
68+ "juju-unit-name": "unit-name",
69+ "juju-api-addresses": "api-addresses",
70+ "juju-private-address": "private-address"}
71+ juju_info = dict.fromkeys(juju_key_map.values())
72+ files_to_delete = []
73+ for filename, info_key in juju_key_map.iteritems():
74+ file_path = os.path.join(meta_data_dir, filename)
75+ if not os.path.exists(file_path):
76+ continue
77+ with open(file_path, "r") as juju_info_part_file:
78+ juju_info[info_key] = juju_info_part_file.read().strip()
79+ files_to_delete.append(file_path)
80+ missing_values = [value is None for value in juju_info.itervalues()]
81+ # If they're all missing it probably means we've already converted.
82+ if all(missing_values):
83+ return 0
84+ # ... otherwise there's something unexpected going on.
85+ if any(missing_values):
86+ return "Incomplete Juju information found in %s" % meta_data_dir
87+ write_json_file(juju_info_file, juju_info)
88+ for file_to_delete in files_to_delete:
89+ os.remove(file_to_delete)
90+ return 0
91+
92+
93 HOOKS = {
94 "config-changed": config_changed,
95+ "upgrade-charm": upgrade_charm,
96 "registration-relation-joined": registration_relation,
97 "registration-relation-changed": registration_relation,
98 "registration-relation-departed": clear_registration,
99
100=== added file 'hooks/test_hooks.py'
101--- hooks/test_hooks.py 1970-01-01 00:00:00 +0000
102+++ hooks/test_hooks.py 2013-10-14 07:53:20 +0000
103@@ -0,0 +1,85 @@
104+import json
105+import os
106+from shutil import rmtree
107+from tempfile import mkdtemp
108+from unittest import TestCase
109+
110+from landscape.deployment import Configuration
111+from hooks import migrate_old_juju_info
112+
113+
114+class MigrateOldJujuInfoTest(TestCase):
115+
116+ def setUp(self):
117+ self.config = Configuration()
118+ self.config.data_path = mkdtemp(prefix=self.__class__.__name__)
119+ self.config.meta_data_path = os.path.join(
120+ self.config.data_path, "metadata.d")
121+ self.addCleanup(rmtree, self.config.data_path)
122+ os.makedirs(self.config.meta_data_path)
123+ super(MigrateOldJujuInfoTest, self).setUp()
124+
125+ def setup_old_juju_info(self, juju_info):
126+ for filename, contents in juju_info.iteritems():
127+ juju_path = os.path.join(self.config.meta_data_path, filename)
128+ with open(juju_path, "w") as juju_file:
129+ juju_file.write(contents)
130+
131+ def test_successful_migration(self):
132+ """
133+ Juju info in an old metadata.d directory is converted into a
134+ single JSON file.
135+ """
136+ juju_info = {"juju-env-uuid": "0afcd28d-6263-43fb-8131-afa696a984f8",
137+ "juju-unit-name": "postgresql/0",
138+ "juju-api-addresses": "10.0.3.1:17070",
139+ "juju-private-address": "10.0.3.205"}
140+ self.setup_old_juju_info(juju_info)
141+ migrate_old_juju_info(self.config)
142+ juju_info_path = os.path.join(self.config.data_path, "juju-info.json")
143+ self.assertTrue(os.path.exists(juju_info_path))
144+ with open(juju_info_path) as juju_info_fh:
145+ juju_json_info = json.load(juju_info_fh)
146+ self.assertEqual(
147+ {"environment-uuid": "0afcd28d-6263-43fb-8131-afa696a984f8",
148+ "unit-name": "postgresql/0",
149+ "api-addresses": "10.0.3.1:17070",
150+ "private-address": "10.0.3.205"}, juju_json_info)
151+
152+ def test_old_juju_info_deleted_after_successful_migration(self):
153+ """
154+ Old Juju files are deleted when the information is succesfully
155+ converted.
156+ """
157+ juju_info = {"juju-env-uuid": "0afcd28d-6263-43fb-8131-afa696a984f8",
158+ "juju-unit-name": "postgresql/0",
159+ "juju-api-addresses": "10.0.3.1:17070",
160+ "juju-private-address": "10.0.3.205"}
161+ self.setup_old_juju_info(juju_info)
162+ migrate_old_juju_info(self.config)
163+ self.assertEqual([], os.listdir(self.config.meta_data_path))
164+
165+ def test_incomplete_juju_info_is_error(self):
166+ """
167+ If only part of the Juju info is in meta-data.d directory then the
168+ function returns non-zero and the files aren't deleted.
169+ """
170+ juju_info = {"juju-env-uuid": "0afcd28d-6263-43fb-8131-afa696a984f8",
171+ "juju-api-addresses": "10.0.3.1:17070",
172+ "juju-private-address": "10.0.3.205"}
173+ self.setup_old_juju_info(juju_info)
174+ self.assertNotEqual(0, migrate_old_juju_info(self.config))
175+ self.assertGreater(len(os.listdir(self.config.meta_data_path)), 0)
176+
177+ def test_idempotent(self):
178+ """
179+ Running the upgrade code more than once has no effect and is not
180+ an error.
181+ """
182+ juju_info = {"juju-env-uuid": "0afcd28d-6263-43fb-8131-afa696a984f8",
183+ "juju-unit-name": "postgresql/0",
184+ "juju-api-addresses": "10.0.3.1:17070",
185+ "juju-private-address": "10.0.3.205"}
186+ self.setup_old_juju_info(juju_info)
187+ migrate_old_juju_info(self.config)
188+ self.assertEqual(0, migrate_old_juju_info(self.config))
189
190=== added symlink 'hooks/upgrade-charm'
191=== target is u'hooks.py'
192=== modified file 'revision'
193--- revision 2013-07-10 08:51:58 +0000
194+++ revision 2013-10-14 07:53:20 +0000
195@@ -1,1 +1,1 @@
196-12
197+13

Subscribers

People subscribed via source and target branches