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

Added support for loading files as parameter values, including interpolation #185

Merged
merged 10 commits into from
Sep 5, 2016
Merged

Added support for loading files as parameter values, including interpolation #185

merged 10 commits into from
Sep 5, 2016

Conversation

ttarhan
Copy link
Contributor

@ttarhan ttarhan commented Aug 3, 2016

Adds a translator to take a filename and return the file contents, optionally encoding or interpolating the input

Fields should use the following format:

<codec>:<path>

For example:

# We've written a file to /some/path:
$ echo "hello there" > /some/path

# In stacker we would reference the contents of this file with the following
conf_key: !file plain:file://some/path

# The above would resolve to
conf_key: hello there

# Or, if we used wanted a base64 encoded copy of the file data
conf_key: !file base64:file://some/path

# The above would resolve to
conf_key: aGVsbG8gdGhlcmUK

Supported codecs:
- plain
- base64 - encode the plain text file at the given path with base64 prior to returning it
- parameterized - the same as plain, but additionally supports referencing template parameters to create userdata that's supplemented with information from the template, as is commonly needed in EC2 UserData.

For example, given a template parameter of BucketName, the file could contain the following text:

     #!/bin/sh
     aws s3 sync s3://{{BucketName}}/somepath /somepath

and then you could use something like this in the YAML config file:

     UserData: !file parameterized:/path/to/file

resulting in the UserData parameter being defined as:

     { "Fn::Join" : ["", [
       "#!/bin/sh\naws s3 sync s3://",
       {"Ref" : "BucketName"},
       "/somepath /somepath"
     ]] }

 - parameterized-b64 - the same as parameterized, with the results additionally
   wrapped in { "Fn::Base64": ... } , which is what you actually need for
   EC2 UserData

When using parameterized-b64 for UserData, you should use a local_parameter defined as such:

  "UserData": {
    "description": "Instance user data",
    "default": None
  }

and then assign UserData in a LaunchConfiguration or Instance to self.local_parameters["UserData"]

… for b64 and UserData-friendly Fn::Join blocks
@ttarhan
Copy link
Contributor Author

ttarhan commented Aug 4, 2016

All - I don't know why the tests above are failing. They are seemingly unrelated to my changes, and caused by util.pt still importing a boto2 module?

@phobologic
Copy link
Member

Hey @ttarhan - this looks cool, thanks! A couple things:

  • the tests are breaking, originally this was due to the cache and an old install of troposphere (I need to figure out what the right thing to do here is going forward - probably locking to specific versions). I re-ran without the cache, and it broke again- this time because of the recent move to boto3, which missed a few hooks (@acmcelwee - not sure if you can look at this, if not I'll dive into it soon). As it is now, I think this is fine @ttarhan.
  • Can you write some tests for the translator? We need some for the kms translator as well (harder to work with, but at least we could write some tests for the utility functions we use from base) - I'll try to get to this.
  • Can you update https://github.com/remind101/stacker/blob/master/docs/translators.rst with this new translator? Might require an update to the formatting, now that we have more than one translator :)

Thanks for this - I'll dig a bit more into the code soon, but after a quick once over it looks fine to me.

@ttarhan
Copy link
Contributor Author

ttarhan commented Aug 4, 2016

Thanks for the quick review, @phobologic. I'll work on a test and the doc update, and update the PR soon.

@phobologic
Copy link
Member

Cool - thanks. I've opened #187 to track the issue w/ the boto2 usage, hope to get that cleared up soon. I also opened #188 to deal with the troposphere mixmatch between stacker & stacker_blueprints. Any comments you have there would be appreciated.

@phobologic
Copy link
Member

hey @ttarhan - when you have a chance, can you merge master into your PR here? It should fix the testing issue you're hitting.

@ttarhan
Copy link
Contributor Author

ttarhan commented Aug 4, 2016

Good morning, @phobologic -- that seems to have done the trick

@phobologic
Copy link
Member

Awesome - thanks to @acmcelwee for the quick fix. We'll be working on one that lets us ditch boto2 more fully again in the future :)

@phobologic
Copy link
Member

Hey @ttarhan - just wanted to ping you and remind you that this PR only needs the documentation & tests. I'm actually almost done fixing the boto3/boto2 issue in #192, which will also involve adding some more libraries for testing, in case those will help you here too. I'm planning to add:

http://pythonhosted.org/testfixtures/ - In my use case I'm planning to use the LogCapture feature.
https://github.com/spulec/moto - mocked out AWS calls (works with boto3 so far)

I should have that PR finished up today.

@ttarhan
Copy link
Contributor Author

ttarhan commented Aug 10, 2016

@phobologic - yeah, as soon as I get a free moment; should be this week.

@phobologic
Copy link
Member

Awesome, thanks :)

@phobologic
Copy link
Member

Oh, one other thing in regards to #188 - can you go ahead and bump the troposphere requirement in setup.py to troposphere>=1.6.0 since the GenericHelperFn class was introduced there? That should fix the tests without needing to worry about the cache. Thanks!

@ttarhan
Copy link
Contributor Author

ttarhan commented Aug 15, 2016

@phobologic - I added some tests. Please see if these are to your liking; Python isn't my everyday specialty, so I apologize for any issues there.



class TestFileTranslator(unittest.TestCase):
def setUp(self):
Copy link
Member

Choose a reason for hiding this comment

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

If you pass on this, you can go ahead and just remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha, oops. I thought I'd use it, so I put it there, and never removed it when it went unused.

Copy link
Member

Choose a reason for hiding this comment

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

I've done that plenty of times myself :)

@phobologic
Copy link
Member

Looks good! If you could go ahead and remove the setUp method that is unused, and then update the docs at https://github.com/remind101/stacker/blob/master/docs/translators.rst to detail this, I'll go ahead and accept it. Thanks again for the PR!

@phobologic
Copy link
Member

Ping @ttarhan - it looks like there are some conflicts now (though they're probably minor). Can you refresh from master, and add to the docs/remove the unused setUp and I'll go ahead and merge this? Hoping to get it into the next release.

@mwildehahn
Copy link
Contributor

Hey @ttarhan, I hate to pull the rug from under this PR, but we've deprecated translators in favor of a more powerful concept called "Lookups". You can see how we converted the kms translator to a lookup here: #200.

For some background on the change you can read through here: #194.

The conversion should mostly just entail moving: stacker/config/translators/file.py to stacker/lookups/handlers/file.py and adding a TYPE_NAME and export a handler.

Let me know if you have any questions!

@ttarhan
Copy link
Contributor Author

ttarhan commented Aug 30, 2016

Ok. I'll do this tomorrow.

@phobologic
Copy link
Member

Hey @ttarhan - I'm tagging this with the milestone of the .8 release. This and #206 will both be the last non-bug PRs going into that release. Thanks for working on it :)

@ttarhan
Copy link
Contributor Author

ttarhan commented Sep 1, 2016

@phobologic and @mhahn: I've got this basically ready, but issues #214 and #213 are preventing me from finishing this change (from translators to lookups). I'm not sure how to address them, especially #214. Let me know where we go from here.

@ttarhan
Copy link
Contributor Author

ttarhan commented Sep 1, 2016

@phobologic and @mhahn - I've updated my branch with my progress, but this isn't ready to merge due to #214. Also, can you confirm my change in stacker/util.py looks right -- I'm shocked nobody else ran into an issue with that import.

@mwildehahn
Copy link
Contributor

ah @ttarhan good catch with the import. when i moved those functions over to utils i didn't change the import path and i haven't used this anywhere i use that function yet.

@mwildehahn
Copy link
Contributor

@ttarhan should be good with: #216

@phobologic
Copy link
Member

With #214 out of the way, is this good to go? (other than fixing the conflicts w/ master?) @ttarhan

@ttarhan
Copy link
Contributor Author

ttarhan commented Sep 3, 2016

We should be. I'll test and report back shortly.

@ttarhan
Copy link
Contributor Author

ttarhan commented Sep 5, 2016

You know, one more thought: we probably want to enhance the interpolation inside the parameterized codec here to support all variables. It currently assumes that any {{Variable}} is a CFN Param (and that was a fair assumption for the most part, until the new variable functionality). Now, we probably need to support both types of interpolation. But perhaps merge this PR and I'll do that work in a follow-up?



class TestFileTranslator(unittest.TestCase):
def setUp(self):
Copy link
Member

Choose a reason for hiding this comment

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

You can remove this, since you pass.

@phobologic
Copy link
Member

Other than the setUp method, this looks great. I agree it'd probably be worth it to combine this with the new variable functionality as well, but that can come after we release .8

@ttarhan
Copy link
Contributor Author

ttarhan commented Sep 5, 2016

Ok, removed the setUp method

@phobologic phobologic merged commit 270f36b into cloudtools:master Sep 5, 2016
@phobologic
Copy link
Member

Thanks, and thanks for sticking with this as we move towards Variables/Lookups.

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.

3 participants