Skip to content
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

Support CloudFormation Parameters within Blueprint Variables #206

Merged

Conversation

mwildehahn
Copy link
Contributor

@mwildehahn mwildehahn commented Aug 31, 2016

Initial pass at fixing: #205.

It isn't as easy to understand as I'd like when reading through the code, but that's because we have so many different references to parameters all over the place from the old parameters. When we remove those this should be much simpler to follow.

This PR introduces a new concept of Blueprint variable "types". These can be used as the type value when defining a Variable the Blueprint accepts.

Right now all of the types are instances of CFNType which will cause the variable to be submitted as a CloudFormation Parameter when launching the stack.

I was going to exclude these from get_variables but I didn't see a downside to including it. I was thinking we might even be able to do something cool like within a blueprint be able to do something like:

from stacker.blueprints.base import Blueprint
from stacker.blueprints.types import CFNString


class SampleBlueprint(object):
    VARIABLES = {
         "CFNParameter": {
             "type": CFNString,
             "description": "A variable that will be submitted as a CFN Parameter",
          },
     }

     def create_template(self):
          t = self.template
          variables = self.get_variables()
          t.add_output(Output("CFNParameter", variables["CFNParameter"]))

Where calling variables["CFNParameter"] will resolve to a troposphere Ref value automatically? The implicit dependency of doing Ref("CFNParameter") always bothered me.


:class`CFNType`` can be used as the `type` for a Blueprint variable.
Unlike other variables, a variable with `type` :class:`CFNType`, will
not be available to the Blueprint while rendering via `get_variables`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this isn't true anymore

Copy link
Member

Choose a reason for hiding this comment

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

Cool, yeah, I like your thoughts around this.

@troyready
Copy link
Contributor

I ran it through a quick test and it looks good to me. More than functional enough as is (would be kinda cool to have the automatic transform to Ref()s as you mentioned but I also could see value in the simplicity of not having that).

Thanks again!


This allows us to filter out non-CloudFormation Parameters from
Blueprint variables when we submit the CloudFormation parameters.

Copy link
Member

Choose a reason for hiding this comment

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

This needs a Args docstring for value, it's a specific type, right?

@phobologic
Copy link
Member

Minor nits, but this looks awesome and is going to provide a lot of flexibility. THanks for jumping on it so quickly. @troyready @acmcelwee might want to take a look at this.

Also, needs docs, but I figure you were waiting to get folks to look at it before documenting.

@acmcelwee
Copy link
Member

Looks like the right direction to me

@phobologic
Copy link
Member

We're down to only 2 PRs, this and #185. As soon as both are complete, I'm going to release a new stacker - I'm thinking we're getting pretty close to what I'd consider 1.0, but I'll probably go with .8 for this one, .9 for the release where we deprecate the old style Parameters entirely, and then 1.0 once that has stabilized for a bit. Sound good?

@mwildehahn
Copy link
Contributor Author

I feel like 1.0.0 should drop parameters since it's a massively backwards incompatible change. I think a 1.0.0 of stacker and stacker_blueprints that moves parameters to variables. Then like 1.1.0 once its stable?

@phobologic phobologic added this to the .8 milestone Aug 31, 2016
@phobologic
Copy link
Member

That seems fair, sounds good to me.

@mwildehahn mwildehahn merged commit 61d954c into cloudtools:master Sep 1, 2016
@mwildehahn mwildehahn deleted the blueprint-variables-cfn-parameters branch September 1, 2016 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants