Added LBaaSv2 volume-based Amphora test #637
Conversation
| """ | ||
| if (openstack_utils.get_os_release() < | ||
| openstack_utils.get_os_release('bionic_train')): | ||
| return |
There was a problem hiding this comment.
It would probably be better to use unittest.skip("reason here") to indicate that the class is being skipped.
There was a problem hiding this comment.
Thank you for reviewing this and for pointing out the corrections I needed to make.
| if 'compute_id' in amphora and server.id == amphora[ | ||
| 'compute_id']: | ||
| server_id = amphora['compute_id'] | ||
| attached_volumes.append(json.dumps(vars(self.nova_client.volumes.get_server_volumes(server_id)))) |
There was a problem hiding this comment.
Is this line over >79 chars (I can't tell from the github review page)?
| def test_create_loadbalancer(self): | ||
| super().test_create_loadbalancer() |
There was a problem hiding this comment.
I'm guessing we need the super() call to make the test actually run (due to inheritance). However, it needs a line above it otherwise I think that the pep8 check should fail. Did you run tox -e pep8?
There was a problem hiding this comment.
Thanks. I'd made a rookie mistake not running the lint tests. The CI caught this
nicholasnjihian
force-pushed
the
test-volume-based-amphora
branch
2 times, most recently
from
a30d4e6 to
b107325
Compare
|
|
||
|
|
||
| @unittest.skipIf(openstack_utils.get_os_release() < | ||
| openstack_utils.get_os_release('bionic_train'), |
There was a problem hiding this comment.
The charm option is introduced from Ussuri https://review.opendev.org/c/openstack/charm-octavia/+/810567.
Is this check supposed to be bionic_ussuri?
There was a problem hiding this comment.
Yes, the feature should be introduced at Ussuri. Not sure why I had this as bionic_train.
Made corrections to PR. Corrected test check to be bionic_ussuri onwards
nicholasnjihian
force-pushed
the
test-volume-based-amphora
branch
from
6d4e6bc to
ab7c28c
Compare
ajkavanagh
added
the
stale
|
@hemanthnakkina-zz @nicholasnjihian Please could you revisit this when you get a moment and update whether it is ready for merge? Please remove the stale label if it's still current. Thanks. |
|
any idea of why is not merged yet? |
|
Closing this PR that's being superseded by #1173 |
Added test for volume-based Amphora.