-
Notifications
You must be signed in to change notification settings - Fork 167
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
Implement Variables and Lookups #194
Implement Variables and Lookups #194
Conversation
These will deprecate PARAMETERS and LOCAL_PARAMETERS. Advantages: - No need to use CF parameters - Leverage yaml lists and dicts - Blueprint subclasses can easily add/overwrite parameters
"""Resolve the values of the blueprint parameters. | ||
|
||
resolve_parameters will resolve the values of the | ||
`BLUEPRINT_PARAMETERS` with values from the command lie, the env file, |
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.
typo
Now supports doing the following:
|
dict: parameters available to the template | ||
|
||
""" | ||
if self.resolved_parameters is None: |
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.
Why not make this do the parameter resolution when it's called if resolved_parameters is None?
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 didn't want get_parameters
to take any arguments and was invoking resolve_parameters
where we were before: https://github.com/remind101/stacker/pull/194/files#diff-92ed81f5017042692188db7b5961c619R203. We need stack.parameters
, provider
and context
which was readily available there, i'm not sure if we can get all of those values when we init the blueprint to make this resolve when called.
👍 other than the small comments I made. Thanks! |
provider = MagicMock() | ||
context = MagicMock() | ||
provider.get_output.return_value = "Test Output" | ||
output = resolve_string("some-stack::Output, other-stack::Output", |
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.
We should ditch this old syntax when using blueprint_parameters
in preference for the new template syntax.
One thing I thought of - what if instead of |
we talked about this a little on slack, but to repeat for the record: my concern with this is just that it gets confusing when the translator gets resolved. right now translators get resolved after the config is parsed, whereas outputs need to be resolved at build time. i think it would get confusing to have some translators resolved pre-build and others during build. |
Curious how this change would impact the generated templates' CFN parameters? Sounds like there won't be any, so I'm wondering if we could still add them to templates (using troposphere.add_parameter) while still using this new functionality? It may be an errand of folly, but I'd like to keep adding CFN parameters for templates that I might share in built json form. |
@troyready The idea was to eventually remove stacker support for parameters & local parameters in favor of these. That wouldn't mean that you couldn't still add them directly via troposphere, it just wouldn't have all the helper logic around building them into the blueprints. The reason for that is that using real CFN parameters in stacker has sort of proven to be pointless. In theory you use parameters to make the templates re-usable, but in stacker you never actually re-use the templates (at least in the use cases I've seen - different environments usually have different buckets/namespaces - we end up with copies of the same template in two different buckets). Do you often share the generated templates with others? |
It's definitely not something used in a normal stacker workflow, but it is nice to be able to share the json templates at times. It's something that I've done with colleagues (e.g. "even though you don't have stacker setup, you can just grab the template from this stack and use it with regular CFN"). Don't get me wrong, I'm not trying to argue for some crazy workflow that depends on that. I just have some reservation about a change that significantly alters what stacker can output (maybe that does mean I depend on spacebar heating?). |
@troyready 😆 re: https://xkcd.com/1172/ We're now calling these |
After talking with @phobologic about the initial implementation of blueprint parameters, we decided to go with a concept of "variables" that wouldn't be as easy to confuse with CF parameters. Variables are only related to how you can build templates within stacker. This PR now introduces two new classes to stacker:
A A Custom lookups can be registered within the config: https://github.com/remind101/stacker_blueprints/pull/24/files#diff-8f84670404b5e0466057d169e260c840R58, https://github.com/remind101/stacker_blueprints/pull/24/files#diff-7cb05c06f16d1eea2c363ecf8e0d1463R3 They can also be nested: https://github.com/remind101/stacker_blueprints/pull/24/files#diff-8f84670404b5e0466057d169e260c840R207 When we parse the config, we register any custom lookups that have been defined. At build time, we resolve all of the variables, invoking the lookup handlers that have been defined. When the In the near future, we'll collapse the concept of "translators" into lookups given that their execution at build time and access to |
@@ -98,13 +104,15 @@ class Blueprint(object): | |||
template. | |||
|
|||
""" | |||
def __init__(self, name, context, mappings=None): | |||
|
|||
def __init__(self, name, context, variables=None, mappings=None): |
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 isn't used
raise UnresolvedVariable(self, variable) | ||
|
||
if variable.value is None: | ||
logger.debug("Got None value for parameter %s, not submitting " |
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.
s/parameter/variable
env file, the config, and any lookups resolved. | ||
|
||
Args: | ||
variables (list): list of 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 a list of a specific class, right? Can you use the :class:
syntax to point at the actual class that you want a list of? Right now it's hard to tell what a list of variables
means.
This is looking so awesome. A couple of nits, and a few questions, but I'm really excited for this. |
Also, once you're feeling comfortable with this, can you add some documentation? Thanks! |
going to add docs in a separate PR, merging this in now! |
These will deprecate PARAMETERS and LOCAL_PARAMETERS.
Advantages:
Fixes: #175
I added: remind101/stacker_blueprints@93db0d2 to stacker_blueprints as a POC for showing how we can leverage the new
BLUEPRINT_PARAMETERS