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

Extensions to cfn tool #136

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

etuttle
Copy link

@etuttle etuttle commented Oct 20, 2014

Hi, I made some extensions to the cfn tool to support my workflow with troposphere.

The only change to existing behavior is the --create operation now waits for the stack creation to complete. It tails along the stack event log waiting for a completion event, printing out events along the way. The exit code of the tool will be nonzero if the creation failed.

I also added --delete and --update operations that behave in a similar way.

I also made the tool automatically render .py templates into json before sending to AWS.

Finally, I added a --capability flag which is necessary to create stacks if they use IAM capabilities.

Is troposphere interested in any of this work?

Thanks for the cool project!

@robzienert
Copy link
Contributor

I've had to build similar features; it'd be great to pull these in.

Merge conflict, by the way.

@phobologic
Copy link
Member

Thanks for this - I'm not sure how I didn't notice it till now. Let me take a look - there's a merge conflict, but if everything looks good I'll fix that and merge.

@phobologic
Copy link
Member

Ok, I took a look - one of the things I think would be good is if it only did the 'tail' of events if you added that flag. It just seems odd to force that functionality on people with create/delete/update when it hasn't existed before.

@etuttle
Copy link
Author

etuttle commented Dec 19, 2014

My main objective was not so much the printing of events by default. I want the capability to submit a request to aws and then wait for it to complete, and exit with an appropriate result code. This behavior is very handy for automation, and makes enough sense that I thought it should be the default. If you just want to submit a CF template and exit immediately, the aws cli can do that already.

It sounds like you're proposing to change the behavior of --tail instead. It currently goes forever whether you combine it with --create or not (which kind of makes sense to me - it works like tail -f).

If backwards compatibility is strictly important, one option would be to add a '--wait' flag that would cause any operation to wait for completion on the aws side.

In summary, the options I can think of are:

  • Change the behavior of --create to wait (my preference)
    • maybe add a --no-wait flag to restore existing behavior
    • maybe add a --quiet flag to not print events while waiting
  • Change the behavior of --tail to mean 'output events and exit when the operation is finished'
  • Add a --wait flag

Thoughts?

Thank you for looking at this!

@phobologic
Copy link
Member

Ahh, ok - I did misunderstand. I think the last option is the right one, just so we don't change the current functionality on anyone. Sorry for taking so long to respond - the holidays had me traveling quite a bit.

Thanks!

@phobologic
Copy link
Member

Hey @etuttle - we actually had someone ask about this change on the cloudtools-dev mailing list. I realize this probably fell off your plate in all this time, but just wanted to check and see if you were working on this still or planned to? Thanks again!

@etuttle
Copy link
Author

etuttle commented May 31, 2015

I'm not actively using it. Moved on to other things and not even using AWS at the moment. But let me take a look at how hard it will be to finish up...

@phobologic
Copy link
Member

NP Ethan - if it proves to be too much of a pain let me know and I'll see what I can do. Thanks!

Do not wait for create, update, or delete operations to complete
without the --wait flag.  See cloudtools#136.
@etuttle
Copy link
Author

etuttle commented May 31, 2015

I rebased the existing commits and then added the --wait flag we discussed back in December. Then force pushed.

Warning, this isn't very well tested! I ran a cfn --wait --capability iam --create <py file> and it appeared to do the right thing, but that was the extent of my testing after the rebase.

@rcuza
Copy link
Contributor

rcuza commented Apr 28, 2016

@phobologic, @etuttle,

I just spent a little time trying to work out the merge conflicts. It looks like the cfn script that is on master has a capabilities command line option like the version in this branch. It would be easy enough to have the changes in this branch squash the file on master.

But as I was getting my fingernails dirty learning the code, I realized there are a number of issues with the script in this branch and the one on master. The version on master has some serious style issue in the create_stack function (e.g. line 36 in commit 3aad426). The version in this branch has at least three pep8 errors. And, I have not even tried to use either version to create a real stack.

It looks like none of the tests touch this script. Which begs the question, should we have anything in this repo of this level of complexity that isn't tested? Also keep in mind that there are other projects, like stacker, that seem to be solving the same problem.

If the answer is yes, these scripts are useful, then I'll create a README to the directory to start the documentation ball rolling for the context of the scripts directory, which will in turn should lead to the next step in resolving this pull request.

@jeanphix
Copy link
Contributor

serious style issue in the create_stack function (e.g. line 36 in commit 3aad426)

@rcuza Why?

@rcuza
Copy link
Contributor

rcuza commented Jul 3, 2016

@jeanphix - It was not consistent with other functions in the script in how it was laid out, but it looks like commit da7e7fc is consistent.

I may of used the serious adjectives because I have a different preference when I align attributes for a function. In hind sight, my preference is not a good enough reason to seriously criticize the style. My apologies. To go into too much detail, 3aad426 commit aligned the closing parenthesis with the 'd' in defand put the attributes at four spaces:

def create_stack(
    conn,
    stackname,
    template=None,
    url=None,
    params=None,
    capabilities=None,
):

I prefer to have the closing parenthesis be aligned with the attributes and the attributes aligned with the opening parenthesis:

def create_stack(conn,
                 stackname,
                 template=None,
                 url=None,
                 params=None,
                 capabilities=None,
                 ):

The most important thing is to be consistent throughout the script.

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.

5 participants