Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
bffccc5
handle additionalProperties in pack config
cognifloyd Apr 8, 2021
46a892c
add pack config with additionalProperties test
cognifloyd Apr 8, 2021
6e864f3
reformat with black
cognifloyd Apr 8, 2021
44e0436
fix defaultdict usage
cognifloyd Apr 8, 2021
2354014
handle defaults under additionalProperties
cognifloyd Apr 8, 2021
9d7c1d5
typo
cognifloyd Apr 8, 2021
3543ad1
mark props with default as not required
cognifloyd Apr 8, 2021
e1ef44b
init additionalProperties in schema for defaults
cognifloyd Apr 9, 2021
46ce281
clean up fixture schema
cognifloyd Apr 9, 2021
8bf7d2a
Changelog entry for pack config additionalProperties support
cognifloyd Apr 9, 2021
a2644c2
Handle defaultdict schema correctly
cognifloyd Apr 9, 2021
b7672ed
Simplify based on PR feedback
cognifloyd Apr 9, 2021
26741cd
Merge branch 'master' into pack-config-additionalproperties
cognifloyd Apr 9, 2021
e69fbae
add clarifying comment in test
cognifloyd Apr 10, 2021
a483888
fix parent_keys in config_loader
cognifloyd Apr 10, 2021
d67b3c9
drop test with unencrypted key in secret: true
cognifloyd Apr 10, 2021
4d3be4a
typo
cognifloyd Apr 10, 2021
574034f
Fix failing test - we need to pass encrypted value to the KVP model.
Kami Apr 10, 2021
d0d9924
Add a temporary workaround to try to retry integration tests which fail
Kami Apr 10, 2021
6bfa390
Add TODO note.
Kami Apr 10, 2021
532e15f
Merge branch 'master' into pack-config-additionalproperties
Kami Apr 10, 2021
1d613bc
Update logging, add timeout-minutes to the step as a safe guard.
Kami Apr 10, 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
2 changes: 1 addition & 1 deletion CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ Added
Contributed by @Kami.

* Fix a bug in the pack config loader so that objects covered by an additionalProperties schema
Comment thread
cognifloyd marked this conversation as resolved.
can use encrypted datastore keys and have their default values applied correctly.
can use encrypted datastore keys and have their default values applied correctly. #5225

Contributed by @cognifloyd.

Expand Down
38 changes: 17 additions & 21 deletions st2common/st2common/util/config_loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
from __future__ import absolute_import
import copy

from collections import defaultdict

import six

from oslo_config import cfg
Expand Down Expand Up @@ -101,16 +99,19 @@ def _get_values_for_config(self, config_schema_db, config_db):
return config

@staticmethod
def _get_object_property_schema(object_schema, init_additional_properties=None):
def _get_object_property_schema(object_schema, additional_properties_keys=None):
"""
Create a schema for an object property using both additionalProperties and properties.

:rtype: ``dict``
"""
property_schema = {}
additional_properties = object_schema.get("additionalProperties", {})
# additionalProperties can be a boolean or a dict
if additional_properties and isinstance(additional_properties, dict):
property_schema = defaultdict(lambda: additional_properties)
else:
property_schema = {}
if init_additional_properties:
# ensure that these keys are present in the object (vs just defaultdict)
for key in init_additional_properties:
property_schema.__missing__(key)
# ensure that these keys are present in the object
for key in additional_properties_keys:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, if that works it should be much more straight forward to understand the code now - well, at least for me :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And re - using copy - I actually verified, if we didn't use copy with defaultdict version, we also don't need to use it here - it's returning by reference version in both scenarios.

And since I believe we indeed never manipulate those values, only read / access them, copy is probably not needed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes. We modify the config object, but we don't modify (and probably should not modify) the schema itself. So, references are perfect in this case.

property_schema[key] = additional_properties
property_schema.update(object_schema.get("properties", {}))
return property_schema

Expand All @@ -135,11 +136,7 @@ def _assign_dynamic_config_values(self, schema, config, parent_keys=None):
for config_item_key, config_item_value in iterator:
if config_is_dict:
# different schema for each key/value pair
try:
# do not use schema.get() as schema might be a defaultdict
schema_item = schema[config_item_key]
except KeyError:
schema_item = {}
schema_item = schema.get(config_item_key, {})
if config_is_list:
# same schema is shared between every item in the list
schema_item = schema
Expand All @@ -150,7 +147,10 @@ def _assign_dynamic_config_values(self, schema, config, parent_keys=None):
# Inspect nested object properties
if is_dictionary:
parent_keys += [str(config_item_key)]
property_schema = self._get_object_property_schema(schema_item)
property_schema = self._get_object_property_schema(
schema_item,
additional_properties_keys=config_item_value.keys(),
)
self._assign_dynamic_config_values(
schema=property_schema,
config=config[config_item_key],
Expand Down Expand Up @@ -216,11 +216,7 @@ def _assign_default_values(self, schema, config):

property_schema = self._get_object_property_schema(
schema_item,
init_additional_properties=(
config[schema_item_key].keys()
if has_additional_properties
else None
),
additional_properties_keys=config[schema_item_key].keys(),
)

self._assign_default_values(
Expand Down