support mixed parameters referencing / not referencing other stacks #183
Conversation
| @@ -89,6 +89,10 @@ def resolve_parameters(parameters, blueprint, context, provider): | |||
| values = value.split(",") | |||
| for v in values: | |||
| v = v.strip() | |||
| # Skip parameters not referencing another stack | |||
There was a problem hiding this comment.
It feels like this logic is in the wrong place - honestly because the original logic was in the wrong place. We should probably pull the for loop that iterates over each part of the Parameter outside of the logic that checks for ::. SOmething like:
v_list = []
values = value.split(",")
for v in values:
... handle regular values, output reference values, etc ...
does that make sense? It'd make it easier, later, to add other special values other than just the :: values as well.
There was a problem hiding this comment.
I also wonder if the whole logic of how we parse the values should be pulled out into it's own function, so we don't have to update it in two places like you had to here.
There was a problem hiding this comment.
I agree. Consolidating that would probably pave the way for the work that's being proposed in #184.
troyready
force-pushed
the
multi_param_types
branch
from
580685d
to
838a858
Compare
troyready
force-pushed
the
multi_param_types
branch
2 times, most recently
from
4abde17
to
ab86f6d
Compare
|
@phobologic good thoughts, I agree. I've added another commit to rework the logic accordingly. I tried to take a stab at consolidating the methods in the 2 files, but gave up for now -- since one of them is only checking for requirements and the other is actually getting stack outputs, I wasn't sure the best way to do it. Open to doing more on it depending on your thoughts. Also, the circleci test is still failing but it doesn't look like it's due to this PR? |
troyready
force-pushed
the
multi_param_types
branch
from
ab86f6d
to
d5ff09c
Compare
|
Hey @troyready, can you try merging master? It's been a while, and there have been fixing changes that might be included in master already that could clear up the Circle errors. So far the code looks good though, I wouldn't owrry about consolidating the methods, I agree with your assessment there. Thanks! |
troyready
force-pushed
the
multi_param_types
branch
2 times, most recently
from
053caf1
to
99d810d
Compare
|
@phobologic thanks for the tip -- I didn't realize it had fallen so behind. I've squashed and rebased it, and everything is looking green now. |
troyready
force-pushed
the
multi_param_types
branch
2 times, most recently
from
4e8a036
to
90e6c3b
Compare
|
@phobologic my cleanup yesterday had introduced a breaking change, sorry about that. I've cleaned that up now and added a test to cover it. |
| v_list.append(str(v).lower()) | ||
| else: | ||
| v_list.append(v) | ||
| if v_list != []: |
There was a problem hiding this comment.
You can get rid of this if statement - if the list is empty, there will be nothing to join, and it'll result in a blank string. Only updating the param if the list is populated is a change from the old behavior, and I'm not sure what issues it might cause.
There was a problem hiding this comment.
@phobologic I think the if does match the old behavior, specifically the way Nones have been handled here.
If a value is always returned, then it trips test_resolve_parameters_none_conversion.
troyready
force-pushed
the
multi_param_types
branch
from
afb3944
to
3ef6e9a
Compare
Currently parameters can reference multiple outputs in other stacks:
```
SomeParameter: stack1::Output1,stack2::Output2
```
This commit allows parameter lists to also include variables not
referencing another stack, e.g.:
```
SomeParameter: ${ThingInEnvFile},stack1::Output1
```
Behind the scenese, now values will always split on commas,
eliminating one of the nested conditionals.
Also changed 2 of the `if`s to `elif`s; probably won't make any
performance impact, but since each condition is independent it feels
more precise to me.
troyready
force-pushed
the
multi_param_types
branch
from
3ef6e9a
to
5f30fee
Compare
|
Commented more specifically on the line comment, but as a broad update just wanted to note that I've rebased on top of master and this should be merge-able now. I can squash the 2nd commit with the new test into a single commit if that'd be better. |
|
Hey @troyready, I think the new lookups Variables and Lookups code (#194) we implemented should resolve your issue without this PR. The new Let me know if that isn't true, closing this for now. |
Currently parameters can reference multiple outputs in other stacks:
This commit allows parameter lists to also include variables not
referencing another stack, e.g.:
I considered updating the documentation in Using Outputs as Parameters, but I'm not sure how to change the last part ("You can also pull multiple...") to note the functionality. Maybe a new section? Maybe the new functionality doesn't need to be called out?