Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate config option existence in testing harness #635

Merged
merged 5 commits into from
Oct 15, 2021
Merged

Validate config option existence in testing harness #635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
416f53e
Initial implementation of ensuring that update_config raises an excep…
jnsgruk Oct 6, 2021
b9abf45
Address feedback; make behaviour more consistent with Juju
jnsgruk Oct 6, 2021
0dc878c
Minor refactor, add a test for unsetting a boolean config item that d…
jnsgruk Oct 7, 2021
d937d73
Merge branch 'main' into validate-config-keys-in-testing
jnsgruk Oct 7, 2021
6bb36ad
Merge branch 'main' into validate-config-keys-in-testing
jnsgruk Oct 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions ops/testing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,8 @@ def __init__(
self._oci_resources = {}
self._framework = framework.Framework(
self._storage, self._charm_dir, self._meta, self._model)
self._update_config(key_values=self._load_config_defaults(config))
self._defaults = self._load_config_defaults(config)
self._update_config(key_values=self._defaults)

@property
def charm(self) -> CharmType:
Expand Down Expand Up @@ -288,8 +289,7 @@ def _load_config_defaults(self, charm_config):
charm_config = dedent(charm_config)
charm_config = yaml.safe_load(charm_config)
charm_config = charm_config.get('options', {})
return {key: value['default'] for key, value in charm_config.items()
if 'default' in value}
return {key: value.get('default', None) for key, value in charm_config.items()}

def add_oci_resource(self, resource_name: str,
contents: typing.Mapping[str, str] = None) -> None:
Expand Down Expand Up @@ -755,8 +755,7 @@ def _update_config(

Args:
key_values: A Mapping of key:value pairs to update in config.
unset: An iterable of keys to remove from Config. (Note that this does
not currently reset the config values to the default defined in config.yaml.)
unset: An iterable of keys to remove from config.
"""
# NOTE: jam 2020-03-01 Note that this sort of works "by accident". Config
# is a LazyMapping, but its _load returns a dict and this method mutates
Expand All @@ -765,9 +764,19 @@ def _update_config(
config = self._backend._config
if key_values is not None:
for key, value in key_values.items():
config[key] = value
if key in self._defaults:
if value is not None:
config[key] = value
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks suspect, as the logic here has changed in more ways than the PR is describing.

Before, the key would always be updated. Now the key will only be updated if the value is not None, which doesn't seem intentional. If that's intentional, why, and if it's not, I think it should be reverted or at least well understood.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is intentional. Per the discussion, we exclude keys with a null value from the config object that the framework tracks. This matches Juju's current behavior.

Copy link
Member Author

@jnsgruk jnsgruk Oct 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, reading this again, this was definitely intentional. is not None specifically allows for people setting someValue: False in the config, which is obviously valid :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see that intentional change being clearly described, neither in code, nor in the PR description, nor in tests. So it at least doesn't look very intentional.

Perhaps I'm missing something?

else:
raise ValueError("unknown config option: '{}'".format(key))

for key in unset:
config.pop(key, None)
# When the key is unset, revert to the default if one exists
default = self._defaults.get(key, None)
if default is not None:
config[key] = default
else:
config.pop(key, None)

def update_config(
self,
Expand Down
8 changes: 8 additions & 0 deletions test/test_model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@ def setUp(self):
resources:
foo: {type: file, filename: foo.txt}
bar: {type: file, filename: bar.txt}
''', config='''
options:
foo:
type: string
bar:
type: int
qux:
type: bool
''')
self.addCleanup(self.harness.cleanup)
self.relation_id_db0 = self.harness.add_relation('db0', 'db')
Expand Down
104 changes: 92 additions & 12 deletions test/test_testing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -773,7 +773,15 @@ def test_update_relation_remove_data(self):
self.assertEqual(viewer.changes, [{'initial': 'data'}, {}])

def test_update_config(self):
harness = Harness(RecordingCharm)
harness = Harness(RecordingCharm, config='''
options:
a:
description: a config option
type: string
b:
description: another config option
type: int
''')
self.addCleanup(harness.cleanup)
harness.begin()
harness.update_config(key_values={'a': 'foo', 'b': 2})
Expand All @@ -794,6 +802,35 @@ def test_update_config(self):
{'name': 'config-changed', 'data': {'a': ''}},
])

def test_update_config_undefined_option(self):
harness = Harness(RecordingCharm)
self.addCleanup(harness.cleanup)
harness.begin()
with self.assertRaises(ValueError):
harness.update_config(key_values={'nonexistent': 'foo'})

def test_update_config_unset_boolean(self):
harness = Harness(RecordingCharm, config='''
options:
a:
description: a config option
type: bool
default: False
''')
self.addCleanup(harness.cleanup)
harness.begin()
# Check the default was set correctly
self.assertEqual(harness.charm.config, {'a': False})
# Set the boolean value to True
harness.update_config(key_values={'a': True})
self.assertEqual(harness.charm.changes, [{'name': 'config-changed', 'data': {'a': True}}])
# Unset the boolean value
harness.update_config(unset={'a'})
self.assertEqual(
harness.charm.changes,
[{'name': 'config-changed', 'data': {'a': True}},
{'name': 'config-changed', 'data': {'a': False}}])

def test_set_leader(self):
harness = Harness(RecordingCharm)
self.addCleanup(harness.cleanup)
Expand Down Expand Up @@ -837,9 +874,18 @@ def test_relation_set_app_not_leader(self):
self.assertEqual(harness.get_relation_data(rel_id, 'test-charm'), {'foo': 'bar'})

def test_hooks_enabled_and_disabled(self):
harness = Harness(RecordingCharm, meta='''
name: test-charm
''')
harness = Harness(
RecordingCharm,
meta='''
name: test-charm
''',
config='''
options:
value:
type: string
third:
type: string
''')
self.addCleanup(harness.cleanup)
# Before begin() there are no events.
harness.update_config({'value': 'first'})
Expand All @@ -861,8 +907,14 @@ def test_hooks_enabled_and_disabled(self):

def test_hooks_disabled_contextmanager(self):
harness = Harness(RecordingCharm, meta='''
name: test-charm
''')
name: test-charm
''', config='''
options:
value:
type: string
third:
type: string
''')
self.addCleanup(harness.cleanup)
# Before begin() there are no events.
harness.update_config({'value': 'first'})
Expand All @@ -883,8 +935,14 @@ def test_hooks_disabled_contextmanager(self):

def test_hooks_disabled_nested_contextmanager(self):
harness = Harness(RecordingCharm, meta='''
name: test-charm
''')
name: test-charm
''', config='''
options:
fifth:
type: string
sixth:
type: string
''')
self.addCleanup(harness.cleanup)
harness.begin()
# Context manager can be nested, so a test using it can invoke a helper using it.
Expand All @@ -896,8 +954,14 @@ def test_hooks_disabled_nested_contextmanager(self):

def test_hooks_disabled_noop(self):
harness = Harness(RecordingCharm, meta='''
name: test-charm
''')
name: test-charm
''', config='''
options:
seventh:
type: string
eighth:
type: string
''')
self.addCleanup(harness.cleanup)
harness.begin()
# If hooks are already disabled, it is a no op, and on exit hooks remain disabled.
Expand Down Expand Up @@ -955,13 +1019,14 @@ def test_config_from_directory(self):
harness = self._get_dummy_charm_harness(tmp)
self.assertEqual(harness.model.config['opt_str'], 'val')
self.assertEqual(harness.model.config['opt_str_empty'], '')
self.assertIsNone(harness.model.config['opt_null'])
self.assertIs(harness.model.config['opt_bool'], True)
self.assertEqual(harness.model.config['opt_int'], 1)
self.assertIsInstance(harness.model.config['opt_int'], int)
self.assertEqual(harness.model.config['opt_float'], 1.0)
self.assertIsInstance(harness.model.config['opt_float'], float)
self.assertNotIn('opt_no_default', harness.model.config)
self.assertFalse('opt_null' in harness.model.config)
self.assertIsNone(harness._defaults['opt_null'])
jnsgruk marked this conversation as resolved.
Show resolved Hide resolved
self.assertIsNone(harness._defaults['opt_no_default'])

def test_set_model_name(self):
harness = Harness(CharmBase, meta='''
Expand Down Expand Up @@ -1362,6 +1427,11 @@ def test_get_pod_spec(self):
def test_begin_with_initial_hooks_no_relations(self):
harness = Harness(RecordingCharm, meta='''
name: test-app
''', config='''
options:
foo:
description: a config option
type: string
''')
self.addCleanup(harness.cleanup)
harness.update_config({'foo': 'bar'})
Expand All @@ -1382,6 +1452,11 @@ def test_begin_with_initial_hooks_no_relations(self):
def test_begin_with_initial_hooks_no_relations_not_leader(self):
harness = Harness(RecordingCharm, meta='''
name: test-app
''', config='''
options:
foo:
description: a config option
type: string
''')
self.addCleanup(harness.cleanup)
harness.update_config({'foo': 'bar'})
Expand All @@ -1408,6 +1483,11 @@ def __init__(self, framework):
peers:
peer:
interface: app-peer
''', config='''
options:
foo:
description: a config option
type: string
''')
self.addCleanup(harness.cleanup)
harness.update_config({'foo': 'bar'})
Expand Down