Bug/1828424 #156

addyess · 2020-01-21T20:48:14Z

Port keystone-ldap basic tests to zaza

zaza/openstack/charm_tests/keystone/tests.py

ajkavanagh

Please see inline comments.

zaza/openstack/charm_tests/keystone/tests.py

addyess · 2020-01-22T15:36:40Z

zaza/openstack/charm_tests/keystone/tests.py

+            )
+
+        with self.config_change(
+                self.config_current(application_name),


here's the use of a new method config_current

addyess · 2020-01-22T15:37:55Z

zaza/openstack/charm_tests/test_utils.py

@@ -133,6 +133,29 @@ def setUpClass(cls, application_name=None, model_alias=None):
            model_name=cls.model_name)
        logging.debug('Leader unit is {}'.format(cls.lead_unit))

+    def config_current(self, application_name=None, keys=None):


new method here to get the current config from an application. You can supply keys if you only want certain items from the current config.

ajkavanagh

Some really nice, clean, code in this patchset. There's just one minor change to a docstring and it's good to go.

zaza/openstack/charm_tests/test_utils.py

ajkavanagh · 2020-01-23T09:34:12Z

Hi Adam

I not sure, but it looks like you've switched all the docstring params/types to @something: from :something: .. the latter (:something:) is more correct. We used to use @param in Juju and older code, but we switched to the current form as it's more widespread/accepted by the Python community. I realise it's confusing! (particularly when you look at charm-helpers and see that both styles are in use!)

addyess · 2020-01-23T16:51:00Z

force of habit. I've been doing @param: for too long. Heck i may even have my editor set-up that way.

I'll fix that up

ajkavanagh

LGTM - thanks for doing the changes.

addyess · 2020-01-24T17:26:02Z

zaza/openstack/charm_tests/keystone/tests.py

+        :rtype: Tuple[bool, Dict[str,str]]
+        """
+        ldap_ips = zaza.model.get_app_ips("ldap-server")
+        self.assertTrue(ldap_ips, "Should be at least one ldap server")


@ajkavanagh @thedac let's assert if the ldap-server isn't there.

thedac

A couple of nits. And a need for more discussion.

I did not realize until I got to the end that you were just moving existing code to create the config_current method. I am not sure I love this in its original form, but I concede it exists that way already. I would really like a second opinion on this before landing.

thedac · 2020-01-24T18:17:08Z

zaza/openstack/charm_tests/keystone/tests.py

+            )
+            states = {
+                'keystone': {
+                    'workload-status': 'idle',


There is another method for checking for idleness. workload-state should be active here. This is, however, the default, making the setting of states unecessary.
The best practice is to check the tests.yaml which allows future developers to change expectations:
test_config = lifecycle_utils.get_charm_config(fatal=False)
zaza.model.wait_for_application_states(
states=test_config.get("target_deploy_status", {}))

thedac · 2020-01-24T18:21:08Z

zaza/openstack/charm_tests/test_utils.py

+        # convert the more elaborate config structure from libjuju to key-value
+        keys = keys or _app_config.keys()
+        # note that conversion to string for all values is due to
+        # attempting to set any config with other types lead to Traceback


s/lead/leads
Though the whole sentence could be clearer.

Beyond the comment, I am not sold on changing the types. I would be interested to see the traceback when you don't set things to a string.

if i remember rightly, the traceback was trying to cast a go native type into a python type? That may be why str(*) is used here. Ill see if i can reproduce.

Also, I think I remember that passing the config back with model.set_application_config fussed about the types?

yeah... here's the issue. You can 'get' the current config such that the types are preserved (aka -- not mapped through str) but when you play them back into the model.set_application_config -- it doesn't like anything that isn't str typed.

2020-01-24 12:40:44 [INFO] File "/home/ubuntu/charms/layers/keystone-ldap/build/builds/keystone-ldap/.tox/func-smoke/lib/python3.7/site-packages/zaza/model.py", line 531, in async_set_application_config 2020-01-24 12:40:44 [INFO] .set_config(configuration)) 2020-01-24 12:40:44 [INFO] File "/home/ubuntu/charms/layers/keystone-ldap/build/builds/keystone-ldap/.tox/func-smoke/lib/python3.7/site-packages/juju/application.py", line 383, in set_config 2020-01-24 12:40:44 [INFO] return await app_facade.Set(application=self.name, options=config) 2020-01-24 12:40:44 [INFO] File "/home/ubuntu/charms/layers/keystone-ldap/build/builds/keystone-ldap/.tox/func-smoke/lib/python3.7/site-packages/juju/client/facade.py", line 471, in wrapper 2020-01-24 12:40:44 [INFO] reply = await f(*args, **kwargs) 2020-01-24 12:40:44 [INFO] File "/home/ubuntu/charms/layers/keystone-ldap/build/builds/keystone-ldap/.tox/func-smoke/lib/python3.7/site-packages/juju/client/_client8.py", line 1124, in Set 2020-01-24 12:40:44 [INFO] reply = await self.rpc(msg) 2020-01-24 12:40:44 [INFO] File "/home/ubuntu/charms/layers/keystone-ldap/build/builds/keystone-ldap/.tox/func-smoke/lib/python3.7/site-packages/juju/client/facade.py", line 607, in rpc 2020-01-24 12:40:44 [INFO] result = await self.connection.rpc(msg, encoder=TypeEncoder) 2020-01-24 12:40:44 [INFO] File "/home/ubuntu/charms/layers/keystone-ldap/build/builds/keystone-ldap/.tox/func-smoke/lib/python3.7/site-packages/juju/client/connection.py", line 456, in rpc 2020-01-24 12:40:44 [INFO] raise errors.JujuAPIError(result) 2020-01-24 12:40:44 [INFO] juju.errors.JujuAPIError: json: cannot unmarshal bool into Go struct field ApplicationSet.options of type string

…me documentation issues

…g to zaza.model does required all values are strings

thedac

This looks great.

removing disco_stein as its EOL func-test-pr: openstack-charmers/zaza-openstack-tests#156 Change-Id: Ice0ae709817db2fd9ffcff299ca403b70dd22721 Closes-Bug: #1828424

* Update charm-keystone-ldap from branch 'master' - Migrate keystone-ldap tests to zaza removing disco_stein as its EOL func-test-pr: openstack-charmers/zaza-openstack-tests#156 Change-Id: Ice0ae709817db2fd9ffcff299ca403b70dd22721 Closes-Bug: #1828424

addyess added 2 commits January 21, 2020 14:23

import functional tests keystone-ldap and zaza-ify

ca8e1e9

wait for keystone to settle also

0c9e006

addyess commented Jan 21, 2020

View reviewed changes

zaza/openstack/charm_tests/keystone/tests.py Outdated Show resolved Hide resolved

addyess commented Jan 21, 2020

View reviewed changes

zaza/openstack/charm_tests/keystone/tests.py Outdated Show resolved Hide resolved

addyess requested review from coreycb and thedac January 21, 2020 20:54

ajkavanagh requested changes Jan 22, 2020

View reviewed changes

addyess added 3 commits January 22, 2020 09:30

Integrate suggestions from ajkavanagh

b4f1201

resolve another suggestion from ajkavanagh

59b06a9

Resolve pep8 violations

b75aaa7

addyess commented Jan 22, 2020

View reviewed changes

addyess requested a review from ajkavanagh January 22, 2020 15:39

ajkavanagh reviewed Jan 22, 2020

View reviewed changes

zaza/openstack/charm_tests/test_utils.py Outdated Show resolved Hide resolved

addyess added 3 commits January 22, 2020 13:14

improve docstrings for config_current

e294034

improved docstrings for ldap tests

c76b7d4

improve docstring

873b64c

addyess requested a review from ajkavanagh January 22, 2020 19:25

use ':' style argument definitions rather than '@'

f065050

ajkavanagh approved these changes Jan 24, 2020

View reviewed changes

addyess added 2 commits January 24, 2020 11:23

use ':' style argument definitions rather than '@'

7063056

assert if ldap server isn't available

852a279

addyess commented Jan 24, 2020

View reviewed changes

thedac suggested changes Jan 24, 2020

View reviewed changes

addyess added 4 commits January 24, 2020 12:59

integrate comments from @thedac to improve waiting for stable, and so…

0614ff5

…me documentation issues

config_current doesn't need to flatten values to , but applying confi…

b080f5f

…g to zaza.model does required all values are strings

resolve pep8 issues

056b255

also don't cast config keys to str

abfa960

_stringified config value of --> ''

b7890be

addyess mentioned this pull request Jan 27, 2020

ApplicationFacade doesn't allow set_config with non-string values juju/python-libjuju#388

Closed

thedac approved these changes Jan 27, 2020

View reviewed changes

addyess added 3 commits January 28, 2020 14:03

attempt to force keystone_v3 for ldap tests

416c85b

attempt to force ldap tests to use keystone v3 api

51f7f2c

simplify looking for user from keystone

7f82aa3

coreycb force-pushed the master branch from 19dc714 to 9692dd0 Compare January 31, 2020 15:25

thedac merged commit be3ea8e into openstack-charmers:master Feb 3, 2020

addyess deleted the bug/1828424 branch February 3, 2020 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug/1828424 #156

Bug/1828424 #156

addyess commented Jan 21, 2020

ajkavanagh left a comment

addyess Jan 22, 2020

addyess Jan 22, 2020

ajkavanagh left a comment

ajkavanagh commented Jan 23, 2020

addyess commented Jan 23, 2020

ajkavanagh left a comment

addyess Jan 24, 2020

thedac left a comment

thedac Jan 24, 2020

addyess Jan 24, 2020

thedac Jan 24, 2020

addyess Jan 24, 2020

addyess Jan 24, 2020

thedac left a comment

Bug/1828424 #156

Bug/1828424 #156

Conversation

addyess commented Jan 21, 2020

ajkavanagh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajkavanagh left a comment

Choose a reason for hiding this comment

ajkavanagh commented Jan 23, 2020

addyess commented Jan 23, 2020

ajkavanagh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thedac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thedac left a comment

Choose a reason for hiding this comment