Remove hardcoded nova-compute app name and add ceph keys test #1141
Conversation
rodrigogansobarbieri
force-pushed
the
zaza_nova_compute_refactor
branch
2 times, most recently
from
6109617 to
d832962
Compare
rodrigogansobarbieri
force-pushed
the
zaza_nova_compute_refactor
branch
from
d832962 to
5d7d884
Compare
| @@ -470,10 +481,50 @@ def test_901_pause_resume(self): | |||
| with self.pause_resume(['nova-compute']): | |||
| logging.info("Testing pause resume") | |||
|
|
|||
| def test_904_test_ceph_keys(self): | |||
| """Run pause and resume tests. | |||
There was a problem hiding this comment.
docstring needs to be a proper one instead of this copy-pasted crap
| self.assertNotEqual(v, result[1]) | ||
| key_dict[result[0]] = result[1] | ||
|
|
||
| check_keyring([{self.application_name: [self.application_name]}]) |
There was a problem hiding this comment.
this was coded for stable/yoga where it does not have the partial fix that is on master. This needs to be adjusted to be able to run against the code on master.
There was a problem hiding this comment.
So I was going to ask about this, is the typical argument just a list of dictionaries like:
[{'nova-compute': ['nova-compute']}]
And then L500 above is just iterating through that? Seems it could be simplified if so.
There was a problem hiding this comment.
I rewrote that logic in the new version I am going to push soon
| @@ -372,7 +380,7 @@ def wait_for_nova_compute_count(expected_count): | |||
|
|
|||
| # Wait for nova-compute service to be removed from the | |||
| # nova-cloud-controller | |||
| if not wait_for_nova_compute_count(0): | |||
| if not wait_for_nova_compute_count(service_count-1): | |||
There was a problem hiding this comment.
Based on the (hidden) code above this, service_count must either be 0 or 1, so this change and below aren't actually necessary.
There was a problem hiding this comment.
also need whitespace around the '-'
There was a problem hiding this comment.
that is "future code", removing hardcoded values without changing the behavior, and keeping it ready to support more than 1 unit
| self.assertNotEqual(v, result[1]) | ||
| key_dict[result[0]] = result[1] | ||
|
|
||
| check_keyring([{self.application_name: [self.application_name]}]) |
There was a problem hiding this comment.
So I was going to ask about this, is the typical argument just a list of dictionaries like:
[{'nova-compute': ['nova-compute']}]
And then L500 above is just iterating through that? Seems it could be simplified if so.
| unit, keyring_file)) | ||
|
|
||
| result = regex.findall(data) | ||
| self.assertEqual(2, len(result)) |
There was a problem hiding this comment.
nit: might be more obvious if you pull the key/value out here:
app_name, app_value = result
But just a nit
There was a problem hiding this comment.
It would raise value error if the length is different. The assert validation is less imprecise as an error message.
| result.append(app_name) | ||
| except KeyError: | ||
| # Older juju versions may not have the field "charm-name" | ||
| if charm_name in app_data['charm']: |
There was a problem hiding this comment.
Do older versions return a list here for app_data['charm']? If not then a
simple string comparison such as you used a few lines above would be better.
There was a problem hiding this comment.
the value here could be 'nova-compute-kvm' for example, so we are checking if the 'nova-compute' string is contained in app_data['charm'] string
| } | ||
| for app in ('nova-compute', 'nova-cloud-controller',): | ||
| for app in (nova_compute_app_name, 'nova-cloud-controller',): |
There was a problem hiding this comment.
I might be misunderstanding the intent here, but if you anticipate that there
might be several apps that use the nova-compute charm, wouldn't you want to
restart services on all of those? However, you only store the first application
in line 191.
There was a problem hiding this comment.
yes, that was partial future-proofing code, it is easy to make it fully future proof, making those changes now. I was initially just fixing the error where it was hardcoded.
| if charm_name == app_data['charm-name']: | ||
| result.append(app_name) | ||
| except KeyError: | ||
| # Older juju versions may not have the field "charm-name" |
There was a problem hiding this comment.
I see references to "charm-name" as far back as juju 1.25 and it is definitely in 2.3, maybe this is overkill?
There was a problem hiding this comment.
I have seen juju status json without this field while coding probelius, then a later juju update introduced this field consistently. A bit overkill probably, but harmless I suppose. Given this is a CI code then I guess it is ok to remove and keep only what we expect to be the latest.
rodrigogansobarbieri
requested review from
nicolasbock,
dosaboy and
brianphaley
rodrigogansobarbieri
changed the title
rodrigogansobarbieri
force-pushed
the
zaza_nova_compute_refactor
branch
from
fd40e90 to
76a2b81
Compare
| for unit in zaza.model.get_units('nova-compute', | ||
| model_name=self.model_name): | ||
| for unit in zaza.model.get_units( | ||
| self.application_name, |
There was a problem hiding this comment.
are you sure that self.application_name is being set to the name of the application in the same way as get_app_names_for_charm()?
There was a problem hiding this comment.
it is somewhat similar. The base class code that assigns self.application_name checks if each app starts with the charm_name declared in tests.yaml, and it breaks when it finds the first one. We could remove the break and go on creating a "self.application_nameS" variable and we ultimately have the same as get_app_names_for_charm. Ultimately I'd prefer if it uses "in (contains)" instead of startswith though.
|
Closed the PR as we don't intend to refactor this at the moment |
WIP