-
Notifications
You must be signed in to change notification settings - Fork 312
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.
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
[Resolve #1252] Supporting resolvers in Hook and Resolver arguments, with new !substitute, !join, !split, and !select resolvers! #1313
Changes from 9 commits
cb5a477
382938f
caa6d3f
f34631e
24f3333
5376462
8ba2d02
1b56f5e
9712bfa
617c37a
b7f4059
8ccb8bd
5d14ec0
a98a955
c739663
ee70e4e
868448b
1fc438d
80cb201
01130d4
ac07a92
13cf87d
1502a81
2c42905
aa1a258
62bcb29
51c7079
6f84e48
c5f9c72
9fe80bc
f75e3c3
20ca362
d7eb15f
5ad9748
5888204
d57bb3d
d8cd879
9080e70
26d960e
6babfb8
b3d50ff
e1665ad
fab6544
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,11 +7,13 @@ | |
|
||
from typing import TYPE_CHECKING, Any | ||
|
||
from sceptre.resolvers import ResolvableArgumentBase | ||
|
||
if TYPE_CHECKING: | ||
from sceptre.stack import Stack | ||
|
||
|
||
class Hook(abc.ABC): | ||
class Hook(ResolvableArgumentBase, metaclass=abc.ABCMeta): | ||
""" | ||
Hook is an abstract base class that should be inherited by all hooks. | ||
|
||
|
@@ -20,21 +22,12 @@ class Hook(abc.ABC): | |
""" | ||
|
||
def __init__(self, argument: Any = None, stack: "Stack" = None): | ||
super().__init__(argument, stack) | ||
self.logger = logging.getLogger(__name__) | ||
|
||
if stack is not None: | ||
self.logger = StackLoggerAdapter(self.logger, stack.name) | ||
|
||
self.argument = argument | ||
self.stack = stack | ||
|
||
def setup(self): | ||
""" | ||
setup is a method that may be overwritten by inheriting classes. Allows | ||
hooks to run so initalisation steps when config is first read. | ||
""" | ||
pass # pragma: no cover | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now defined in the base class |
||
@abc.abstractmethod | ||
def run(self): | ||
""" | ||
|
@@ -43,15 +36,6 @@ def run(self): | |
""" | ||
pass # pragma: no cover | ||
|
||
def clone(self, stack: "Stack") -> "Hook": | ||
""" | ||
Produces a "fresh" copy of the Hook, with the specified stack. | ||
|
||
:param stack: The stack to set on the cloned resolver | ||
""" | ||
clone = type(self)(self.argument, stack) | ||
return clone | ||
|
||
Comment on lines
-46
to
-54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now defined on the base class |
||
|
||
class HookProperty(object): | ||
""" | ||
|
@@ -84,7 +68,7 @@ def __set__(self, instance: "Stack", value): | |
""" | ||
|
||
def setup(attr, key, value: Hook): | ||
attr[key] = clone = value.clone(instance) | ||
attr[key] = clone = value.clone_for_stack(instance) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
clone.setup() | ||
|
||
_call_func_on_values(setup, value, Hook) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
from threading import RLock | ||
from typing import Any, TYPE_CHECKING, Type, Union, TypeVar | ||
|
||
from sceptre.helpers import _call_func_on_values | ||
from sceptre.helpers import _call_func_on_values, delete_keys_from_containers | ||
from sceptre.logging import StackLoggerAdapter | ||
from sceptre.resolvers.placeholders import ( | ||
create_placeholder_value, | ||
|
@@ -17,13 +17,113 @@ | |
from sceptre import stack | ||
|
||
T_Container = TypeVar("T_Container", bound=Union[dict, list]) | ||
Self = TypeVar("Self") | ||
|
||
|
||
class RecursiveResolve(Exception): | ||
pass | ||
|
||
|
||
class Resolver(abc.ABC): | ||
class ResolvableArgumentBase: | ||
def __init__(self, argument: Any = None, stack: "stack.Stack" = None): | ||
self.stack = stack | ||
|
||
self._argument = argument | ||
self._argument_is_resolved = False | ||
|
||
@property | ||
def argument(self) -> Any: | ||
"""This is the resolver's argument. | ||
|
||
This property will resolve all nested resolvers inside the argument, but only if this resolver | ||
has a Stack. | ||
|
||
Resolving nested resolvers will result in their values being replaced in the dict/list they | ||
were in with their resolved value, so we won't have to resolve them again. | ||
|
||
If this property is accessed BEFORE the resolver has a stack, it will return | ||
the raw argument value. This is to safeguard any __init__() behaviors from triggering | ||
resolution prematurely. | ||
""" | ||
if self.stack is not None and not self._argument_is_resolved: | ||
self._resolve_argument() | ||
|
||
return self._argument | ||
|
||
@argument.setter | ||
def argument(self, value): | ||
self._argument = value | ||
|
||
def _resolve_argument(self): | ||
"""Resolves all argument resolvers recursively.""" | ||
|
||
keys_to_delete = [] | ||
|
||
def resolve(attr, key, obj: Resolver): | ||
result = obj.resolve() | ||
if result is None: | ||
keys_to_delete.append((attr, key)) | ||
else: | ||
attr[key] = result | ||
|
||
_call_func_on_values(resolve, self._argument, Resolver) | ||
delete_keys_from_containers(keys_to_delete) | ||
|
||
self._argument_is_resolved = True | ||
|
||
def _setup_nested_resolvers(self): | ||
"""Ensures all nested resolvers in this resolver's argument are also setup when this | ||
resolver's setup method is called. | ||
""" | ||
|
||
def setup_nested(attr, key, obj: Resolver): | ||
obj.setup() | ||
|
||
_call_func_on_values(setup_nested, self._argument, Resolver) | ||
|
||
def _clone(self: Self, stack: "stack.Stack") -> Self: | ||
"""Recursively clones the resolver and its arguments. | ||
|
||
The returned resolver will have an identical argument that is a different memory reference, | ||
so that resolvers inherited from a stack group and applied across multiple stacks are | ||
independent of each other. | ||
|
||
Furthermore, all nested resolvers in this resolver's argument will also be cloned to ensure | ||
they themselves are also independent and fully configured for the current stack. | ||
""" | ||
|
||
def recursively_clone(obj): | ||
if isinstance(obj, Resolver): | ||
return obj._clone(stack) | ||
if isinstance(obj, list): | ||
return [recursively_clone(item) for item in obj] | ||
elif isinstance(obj, dict): | ||
return {key: recursively_clone(val) for key, val in obj.items()} | ||
return obj | ||
|
||
argument = recursively_clone(self._argument) | ||
clone = type(self)(argument, stack) | ||
return clone | ||
|
||
def clone_for_stack(self: Self, stack: "stack.Stack") -> Self: | ||
"""Obtains a clone of the current object, setup and ready for use for a given Stack | ||
instance. | ||
""" | ||
clone = self._clone(stack) | ||
clone._setup_nested_resolvers() | ||
clone.setup() | ||
return clone | ||
|
||
def setup(self): | ||
""" | ||
This method is called at during stack initialisation. | ||
Implementation of this method in subclasses can be used to do any | ||
initial setup of the object. | ||
""" | ||
pass # pragma: no cover | ||
|
||
|
||
class Resolver(ResolvableArgumentBase, metaclass=abc.ABCMeta): | ||
""" | ||
Resolver is an abstract base class that should be inherited by all | ||
Resolvers. | ||
|
@@ -33,21 +133,12 @@ class Resolver(abc.ABC): | |
""" | ||
|
||
def __init__(self, argument: Any = None, stack: "stack.Stack" = None): | ||
super().__init__(argument, stack) | ||
|
||
self.logger = logging.getLogger(__name__) | ||
if stack is not None: | ||
self.logger = StackLoggerAdapter(self.logger, stack.name) | ||
|
||
self.argument = argument | ||
self.stack = stack | ||
|
||
def setup(self): | ||
""" | ||
This method is called at during stack initialisation. | ||
Implementation of this method in subclasses can be used to do any | ||
initial setup of the object. | ||
""" | ||
pass # pragma: no cover | ||
|
||
@abc.abstractmethod | ||
def resolve(self): | ||
""" | ||
|
@@ -58,14 +149,6 @@ def resolve(self): | |
""" | ||
pass # pragma: no cover | ||
|
||
def clone(self, stack: "stack.Stack") -> "Resolver": | ||
""" | ||
Produces a "fresh" copy of the Resolver, with the specified stack. | ||
|
||
:param stack: The stack to set on the cloned resolver | ||
""" | ||
return type(self)(self.argument, stack) | ||
|
||
|
||
class ResolvableProperty(abc.ABC): | ||
""" | ||
|
@@ -127,23 +210,6 @@ def _no_recursive_get(self, stack: "stack.Stack"): | |
finally: | ||
setattr(stack, get_status_name, False) | ||
|
||
def get_setup_resolver_for_stack( | ||
self, stack: "stack.Stack", resolver: Resolver | ||
) -> Resolver: | ||
"""Obtains a clone of the resolver with the stack set on it and the setup method having | ||
been called on it. | ||
|
||
:param stack: The stack to set on the Resolver | ||
:param resolver: The Resolver to clone and set up | ||
:return: The cloned resolver. | ||
""" | ||
# We clone the resolver when we assign the value so that every stack gets its own resolver | ||
# rather than potentially having one resolver instance shared in memory across multiple | ||
# stacks. | ||
clone = resolver.clone(stack) | ||
clone.setup() | ||
return clone | ||
|
||
Comment on lines
-130
to
-146
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method is no longer necessary. It's all been moved into the CustomYamlTagBase's |
||
@abc.abstractmethod | ||
def get_resolved_value( | ||
self, stack: "stack.Stack", stack_class: Type["stack.Stack"] | ||
|
@@ -254,20 +320,7 @@ def resolve(attr: Union[dict, list], key: Union[int, str], value: Resolver): | |
|
||
container = getattr(stack, self.name) | ||
_call_func_on_values(resolve, container, Resolver) | ||
# Remove keys and indexes from their containers that had resolvers resolve to None. | ||
list_items_to_delete = [] | ||
for attr, key in keys_to_delete: | ||
if isinstance(attr, list): | ||
# If it's a list, we want to gather up the items to remove from the list. | ||
# We don't want to modify the list length yet. | ||
# Since removals will change all the other list indexes, | ||
# we don't wan't to modify lists yet. | ||
list_items_to_delete.append((attr, attr[key])) | ||
else: | ||
del attr[key] | ||
|
||
for containing_list, item in list_items_to_delete: | ||
containing_list.remove(item) | ||
delete_keys_from_containers(keys_to_delete) | ||
|
||
return container | ||
|
||
|
@@ -294,7 +347,7 @@ def _clone_container_with_resolvers( | |
|
||
def recurse(obj): | ||
if isinstance(obj, Resolver): | ||
return self.get_setup_resolver_for_stack(stack, obj) | ||
return obj.clone_for_stack(stack) | ||
if isinstance(obj, list): | ||
return [recurse(item) for item in obj] | ||
elif isinstance(obj, dict): | ||
|
@@ -389,5 +442,5 @@ def assign_value_to_stack(self, stack: "stack.Stack", value: Any): | |
:param value: The value to set | ||
""" | ||
if isinstance(value, Resolver): | ||
value = self.get_setup_resolver_for_stack(stack, value) | ||
value = value.clone_for_stack(stack) | ||
setattr(stack, self.name, value) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
from sceptre.resolvers import Resolver | ||
|
||
|
||
class Join(Resolver): | ||
def resolve(self): | ||
delimiter, items_list = self.argument | ||
joined = delimiter.join(items_list) | ||
return joined |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
from sceptre.resolvers import Resolver | ||
|
||
|
||
class Select(Resolver): | ||
def resolve(self): | ||
index, items = self.argument | ||
return items[index] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
from sceptre.resolvers import Resolver | ||
|
||
|
||
class Split(Resolver): | ||
def resolve(self): | ||
split_on, split_string = self.argument | ||
return split_string.split(split_on) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
from sceptre.resolvers import Resolver | ||
|
||
|
||
class Substitute(Resolver): | ||
def resolve(self): | ||
template, variables = self.argument | ||
return template.format(**variables) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mostly just extracted out of the ResolvableProperty class and put here. I needed to also use this in the CustomYamlTagBase class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad that this has been extracted but I'm a bit queasy about this function mutating collections it doesn't own. I got side-tracked by work but I feel this could be simplified (in the future) using list comprehensions and/or
filter
. But then I wasn't able to dig enough into how it's used to propose a refactor.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I posted a previous response to this and then I'm replacing it with this, because I thought you were talking about something else.
If you look at how this is used, mutation is exactly the purpose of this function. Filtered lists and dicts will not suit the purpose of what this exists for and won't work. The whole point is in-place modification of containers because of the way that resolvers operate.