[Resolve #1252] Supporting resolvers in Hook and Resolver arguments, with new !substitute, !join, !split, and !select resolvers! #1313

jfalkenstein · 2023-02-25T18:47:18Z

This pull request brings the ability for Hooks and Resolvers to have resolvers in their arguments and have those resolvers automatically resolve when self.argument is accessed. When a resolver or hook is setup, it will also setup all nested resolvers in its argument. And when it is attached to a Stack, it and its argument will be recursively cloned so that no resolver is associated with a different stack. This will let resolvers be "inherited" from parent stack configurations.

PR Checklist

Wrote a good commit message & description [see guide below].
Commit message starts with [Resolve #issue-number].
Added/Updated unit tests.
Added/Updated integration tests (if applicable).
All unit tests (make test) are passing.
Used the same coding conventions as the rest of the project.
The new code passes pre-commit validations (pre-commit run --all-files).
The PR relates to only one subject with a clear title.
and description in grammatically correct, complete sentences.

Approver/Reviewer Checklist

Before merge squash related commits.

Other Information

Guide to writing a good commit

jfalkenstein · 2023-02-26T17:23:03Z

sceptre/hooks/__init__.py

-        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
-


This is now defined in the base class

jfalkenstein · 2023-02-26T17:23:29Z

sceptre/hooks/__init__.py

-    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
-


This is now defined on the base class

jfalkenstein · 2023-03-04T14:30:55Z

sceptre/hooks/__init__.py


 if TYPE_CHECKING:
    from sceptre.stack import Stack


-class Hook(abc.ABC):
+class Hook(CustomYamlTagBase, metaclass=abc.ABCMeta):


The big change here is that, since there's very little that differentiates a hook from a resolver, other than the "resolve" vs "run" public method, they now share a common base class. A lot of the functionality on Hooks has now been moved up into that base class since it was shared with Resolvers.

jfalkenstein · 2023-03-04T14:31:58Z

sceptre/hooks/__init__.py

@@ -84,7 +57,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)


clone_for_stack (as you'll see below) is a recursive clone operation that allows for cloning resolvers inside of the arguments.

dboitnot

I think this is great and will be a boon to users!

Looking at how resolvers work, though, I'm wondering if a more functional approach might drastically improve the code. There's a lot of mutation and behavior that, while correct, is not obvious and might be harder to reason about or troubleshoot.

It feels like resolvers (and maybe hooks?) could be evaluated as a series of transformations that act on data and their invocation is basically an abstract syntax tree.

The correct reply to the above is, of course, "Put your PR where your mouth is Dan." :)

dboitnot · 2023-03-06T14:31:39Z

docs/_source/docs/resolvers.rst

+This resolver allows you to select a specific index of a list of items. This is great for combining
+with the ``!split`` resolver to obtain part of a string. This function works almost the same as
+CloudFormation's ``!Select`` intrinsic function, **except you can use this with negative indexes to
+select with a reverse index**.


What about "except you can use this with negative indices to select from the end of the list." ?

dboitnot · 2023-03-06T14:35:04Z

docs/_source/docs/resolvers.rst

@@ -181,6 +248,32 @@ Example:
   parameters:
     VpcIdParameter: !stack_output_external prj-network-vpc::VpcIdOutput prod

+
+substitute


I don't have super-strong feelings about this, but why "substitute" and not "sub" as in CF? I might argue in favor of "sub" for consistency with CF.

I don't know. I guess I just don't like the abbreviation. It doesn't seem as clear as substitute. @zaro0508 , thoughts on this?

i agree with @dboitnot. Since you are making references to cloudformation intrinsic functions I prefer sub for consistency.

I agree "substitute" has more intrinsic clarity, but since most of our users are going to be pretty familiar with CF and sub I think we could avoid the mental dichotomy by sticking to CF's nomenclature. But I would like @zaro0508's thoughts as well.

I've changed this to sub

dboitnot · 2023-03-06T14:40:03Z

sceptre/helpers.py

+Key = Union[str, int]
+
+
+def delete_keys_from_containers(keys_to_delete: List[Tuple[Container, Key]]):


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.

docs/_source/docs/resolvers.rst

jfalkenstein · 2023-03-06T15:51:57Z

On the subject of in-place mutation...

Because of the cloning employed when a resolver is associated with a stack, it effectively owns the data at that point. The only collections that are mutated are ones that are effectively deep-copied when they are assigned to the stack.
This is generally the way resolvers have always worked. The basic idea is that it prevents us from ever needing to resolve the same resolver twice for the same stack. This can be important, since the resolvable attribute might be accessed a number of times over the course of execution.
Once we've resolved the resolver, there's no sense in keeping it around; its purpose has been fulfilled.

…nto 1252-sub-resolver

quinnjr · 2023-03-11T17:46:45Z

I'm really looking forward to this PR being merged

zaro0508 · 2023-03-06T17:06:30Z

docs/_source/docs/resolvers.rst

+~~~~~~~~~~
+
+This resolver allows you to create a string using Python string format syntax. This functions as a
+great way to combine together a number of resolver outputs into a single string. This functions very


very/is very

zaro0508 · 2023-03-06T17:11:19Z

docs/_source/docs/resolvers.rst

@@ -181,6 +248,32 @@ Example:
   parameters:
     VpcIdParameter: !stack_output_external prj-network-vpc::VpcIdOutput prod

+
+substitute


i agree with @dboitnot. Since you are making references to cloudformation intrinsic functions I prefer sub for consistency.

zaro0508 · 2023-03-11T15:10:45Z

docs/_source/docs/resolvers.rst

+   notifications: !split
+     - ";"
+     - !stack_output my/sns/topics.yaml::SemicolonDelimitedArns


do you think it would make it easier for users to understand if these examples just used direct values? something like this..

notifications: !split - ";" - "note1; note2; note3"

result: the list ["note1", "note2", "note3"] is passed into the notifications parameter

Actually, I don't really. !sub, !join, !select, and !split are ONLY useful when you have resolvers to string together; They're clumsy if you don't. It's better to use Jinja to do these things, if you can. But if you have resolvers whose resolved values you need to manipulate/interpolate/select from, they're really the only way to do that.

As such, If we don't show how these resolvers can be used with other resolvers, we're not really showing how to use them well.

¿Por qué no los dos? :)

notifications: !split - ";" - "note1; note2; note3"

Resolves to:

notifications: [ "note1", "note2", "note3" ]

As a more practical example, the following will split a semicolon-delimited list exported by another stack:

notifications: !split - ";" - !stack_output my/sns/topics.yaml::SemicolonDelimitedArns

@dboitnot, sure, that works. But If you wanted to split a simple string, you'd just go like this:

notifications: {{ "note1; note2; note3".split(";") }}

I fear that if we show people examples of using these resolvers in cases where you don't actually need to use them (and where you really shouldn't use them), it will be more confusing. The only times you need to use these resolvers is when you need to combine/manipulate the outputs of other resolvers. I actually added documentation to that effect to make it absolutely clear.

zaro0508 · 2023-03-11T15:26:07Z

docs/_source/docs/resolvers.rst

+
+   parameters:
+     ConnectionString: !sub
+       - "postgres://{username}:{password}@{hostname}:{port}/{database}"


can the sub reference vars outside of it's scope? Can you set these to build-in vars like account id/region/etc.. What about references to vars from config.yaml If possible it might help users if there was an example or two of those use cases.

!sub needs a second argument that defines the variable values. It's not the same as CloudFormation's sub that lets you just resolve values without defining them. It's just a glorified str.format() invocation. It doesn't have a "scope" outside its own arguments. If you want to resolve something from the Stack scope, you could combine !sub and Jinja syntax together, like this:

!sub # Note the double-curly braces is JINJA syntax to access variables within Jinja scope - "postgres://{{ var.username }}:{password}@{hostname}:{port}/{{ database }}" - password: !ssm /my/ssm/password hostname: !stack_output my/database/stack.yaml::HostName port: !stack_output my/database/stack.yaml::Port

Notice how, if it's accessible within Jinja, you don't actually need to provide that to !sub as a variable, because Jinja interpolation will happen BEFORE the yaml is rendered. I'm going to look into clarifying this in the docs, since I doubt you'll be the first one confused about this.

docs/_source/docs/stack_config.rst

zaro0508 · 2023-03-11T22:43:52Z

tests/test_resolvers/test_select.py

+        return self.argument
+
+
+class TestSplit:


shouldn't this be TestSelect:?

Also would it make sense to have a test to select on a negative index values?

tests/test_resolvers/test_split.py

tests/test_resolvers/test_sub.py

tests/test_resolvers/test_join.py

zaro0508 · 2023-03-11T23:06:07Z

setup.py

            "stack_attr = sceptre.resolvers.stack_attr:StackAttr",
+            "sub = sceptre.resolvers.substitute:Sub",


shouldn't this be sub = sceptre.resolvers.sub:Sub?

Yikes! Yes! That's probably why my tests are failing.

jfalkenstein · 2023-03-12T14:10:08Z

tests/test_resolvers/test_select.py

+        return self.argument
+
+
+class TestSelect:


I need to implement better error handling around missing indexes/keys.

Pydantic has changed my life:

https://docs.pydantic.dev/

zaro0508

generally LGTM. just a few minor fixes and should be good to go.

zaro0508 · 2023-03-20T15:08:14Z

tests/test_resolvers/test_split.py

+        [
+            pytest.param("just a string", id="just a string"),
+            pytest.param([123, "something"], id="first item is not string"),
+            pytest.param(["something", 123], id="first item is not string"),


first/second

zaro0508 · 2023-03-20T15:22:17Z

docs/_source/docs/hooks.rst

@@ -224,7 +226,7 @@ Assume a Sceptre `copy` hook that calls the `cp command`_:
       - !copy
           options: "-r"
           source: "from_dir"
-           destination: "to_dir"
+           destination: !stack_output my/other/stack::CopyDestination


i think the before_update and after_update examples are wrong originally. shouldn't they be like this instead?

before_update: - !copy [ {"options":"-r", "source": "from_dir", "destination": "to_dir"} ] after_update: !copy - options: "-r" source: "from_dir" destination: !stack_output my/other/stack::CopyDestination

No. !copy in this imaginary example takes a dict as an argument, not a list. However, after_update and before_update should be lists. So it's correct in the docs, not in your suggestion.

jfalkenstein added 9 commits February 25, 2023 10:57

creating Sub resolver

cb5a477

creating nestable resolvers

382938f

testing substitute, join, and split

caa6d3f

supporting resolving to nothing

f34631e

adding docstrings and extra test

24f3333

merging nestable resolver with resolver base class

5376462

updating base resolver class

8ba2d02

creating select resolver

1b56f5e

supporting resolvers as hook arguments

9712bfa

jfalkenstein changed the title ~~[Resolve #1252] Supporting Nestable resolvers and introducing !substitute, !join, and !split~~ [Resolve #1252] Supporting resolvers in Hook and Resolver arguments, with new !substitute, !join, !split, and !select resolvers! Feb 26, 2023

testing helper

617c37a

jfalkenstein commented Feb 26, 2023

View reviewed changes

jfalkenstein added 10 commits February 26, 2023 11:45

updating docstrings

b7f4059

documenting new resolvers

8ccb8bd

adding some context to stack config docs

5d14ec0

fixing docs

a98a955

docs tweak

c739663

adding docstrings

ee70e4e

testing for resolvers in resolvers in resolvers

868448b

fixing setup

1fc438d

making an example use sceptre_user_data

80cb201

making example a little more explicit

01130d4

jfalkenstein commented Mar 4, 2023

View reviewed changes

jfalkenstein added 5 commits March 4, 2023 08:34

small docs tweak

ac07a92

improving docstring

13cf87d

improving docstring

1502a81

improving docstring

2c42905

handling resolver situations

aa1a258

jfalkenstein marked this pull request as ready for review March 4, 2023 15:41

dboitnot reviewed Mar 6, 2023

View reviewed changes

jfalkenstein commented Mar 6, 2023

View reviewed changes

docs/_source/docs/resolvers.rst Outdated Show resolved Hide resolved

Apply suggestions from code review

c5f9c72

dboitnot approved these changes Mar 6, 2023

View reviewed changes

jfalkenstein added 2 commits March 11, 2023 08:10

renaming Substitute to Sub

9fe80bc

Merge branch '1252-sub-resolver' of github.com:jfalkenstein/sceptre i…

f75e3c3

…nto 1252-sub-resolver

zaro0508 requested changes Mar 11, 2023

View reviewed changes

jfalkenstein added 6 commits March 12, 2023 08:19

fixing grammer in docs

20ca362

clarifying docs

d7eb15f

making the underline shorter

5ad9748

more robust joining

5888204

fixing test name

d57bb3d

adding better select tests

d8cd879

jfalkenstein commented Mar 12, 2023

View reviewed changes

zaro0508 mentioned this pull request Mar 14, 2023

Allow hooks to calculate their arguments using resolvers #329

Closed

jfalkenstein added 5 commits March 19, 2023 08:45

adding proper error handling around join

9080e70

proper error handling around select

26d960e

testing sub

6babfb8

adding test on invalid argument error raising

b3d50ff

documenting resolvers in a hook argument

e1665ad

jfalkenstein requested a review from zaro0508 March 19, 2023 14:28

zaro0508 requested changes Mar 20, 2023

View reviewed changes

fixing typo in test name

fab6544

zaro0508 approved these changes Mar 24, 2023

View reviewed changes

zaro0508 merged commit d6c3e93 into Sceptre:master Mar 24, 2023

This was referenced Apr 18, 2023

embedded resolvers - resolver ouput as entry paramter for another resolver #1258

Closed

Feature: String multiple resolvers together #1165

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Resolve #1252] Supporting resolvers in Hook and Resolver arguments, with new !substitute, !join, !split, and !select resolvers! #1313

[Resolve #1252] Supporting resolvers in Hook and Resolver arguments, with new !substitute, !join, !split, and !select resolvers! #1313

jfalkenstein commented Feb 25, 2023 •

edited

jfalkenstein Feb 26, 2023

jfalkenstein Feb 26, 2023

jfalkenstein Mar 4, 2023

jfalkenstein Mar 4, 2023

dboitnot left a comment

dboitnot Mar 6, 2023

dboitnot Mar 6, 2023

jfalkenstein Mar 6, 2023

zaro0508 Mar 6, 2023

dboitnot Mar 6, 2023

jfalkenstein Mar 11, 2023

dboitnot Mar 6, 2023

jfalkenstein commented Mar 6, 2023

quinnjr commented Mar 11, 2023

zaro0508 Mar 6, 2023

zaro0508 Mar 6, 2023

zaro0508 Mar 11, 2023

jfalkenstein Mar 12, 2023

dboitnot Mar 13, 2023

jfalkenstein Mar 13, 2023

zaro0508 Mar 11, 2023

jfalkenstein Mar 12, 2023

zaro0508 Mar 11, 2023

jfalkenstein Mar 12, 2023

zaro0508 Mar 11, 2023

jfalkenstein Mar 12, 2023

jfalkenstein Mar 12, 2023

dboitnot Mar 13, 2023

zaro0508 left a comment

zaro0508 Mar 20, 2023

zaro0508 Mar 20, 2023

jfalkenstein Mar 24, 2023

		Key = Union[str, int]


		def delete_keys_from_containers(keys_to_delete: List[Tuple[Container, Key]]):

		"stack_attr = sceptre.resolvers.stack_attr:StackAttr",
		"sub = sceptre.resolvers.substitute:Sub",

[Resolve #1252] Supporting resolvers in Hook and Resolver arguments, with new !substitute, !join, !split, and !select resolvers! #1313

[Resolve #1252] Supporting resolvers in Hook and Resolver arguments, with new !substitute, !join, !split, and !select resolvers! #1313

Conversation

jfalkenstein commented Feb 25, 2023 • edited

PR Checklist

Approver/Reviewer Checklist

Other Information

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

dboitnot 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfalkenstein commented Mar 6, 2023

quinnjr commented Mar 11, 2023

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaro0508 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

jfalkenstein commented Feb 25, 2023 •

edited