Adds a check for REVIEW_IN_PROGRESS on CF stack.

[denmat]

relates to issue #232

Adds a check on REVIEW_IN_PROGRESS status in the Cloudformation stack which can be left if that state by a user or someone else.

[stevehodgkiss]

🔪 whitespace

[stevehodgkiss]

This would be better named without a ? since it isn't used a query method. Something like abort_if_review_in_progress?

Also, rather than return false early, I would add the !stack.nil? && to the if statement. Either that or simply return if stack.nil? since the return value doesn't matter (it'll either fail or return nil or false if stack is not in review currently).

[patrobinson]

Yes! @julianenvato ran into this also yesterday, there are some other edge cases it appears where it can get stuck so I am 💯 for an explicit check to give us a real error rather than the current "fetch is an invalid method for nil class"

Wait a second

[patrobinson]

I don't think REVIEW_IN_PROGRESS is necessarily a bad state? Do we get in this state if we propose a change to an existing stack and select n to applying it?

The main problem we encounter is an empty stack

[denmat]

No, it is not a bad state...but..

The main reason we get this state is if we abort (ctrl c) or don't have perms to delete the stack (after selecting n) - and stack_master doesn't get to clean up after itself which it tries to do after selecting n.

[patrobinson]

We do cleanup if you ctrl c now, but can't if you there are no permissions. I'm just wondering that instead of checking for the status, should we check for an empty stack instead?

[denmat]

hmm here is the result of stack.inspect when we get this issue.

#<StackMaster::Stack:0x00007fd95d50b070 @region="us-east-1", @stack_name="mistack", @stack_id="arn:aws:cloudformation:us-east-1:123456789:stack/mistack-database/6c029cb0-5901-11e8-a5fa-500c2854b699", @parameters={}, @template_body="", @template_format=:yaml, @outputs=[], @role_arn=nil, @notification_arns=[], @stack_policy_body=nil, @stack_status="REVIEW_IN_PROGRESS">

do you think we can test on template_body?

[patrobinson]

@denmat Yep. The issue is not that StackMaster doesn't like the state REVIEW_IN_PROGRESS, just that we have no way to deal with an empty template body.

[denmat]

So I've been digging a bit and found that this was the cause:

stack_master/lib/stack_master/stack.rb

Line 20 in 6c9459f

TemplateUtils.template_hash(template).fetch('Parameters', {}).inject({}) do |result, (parameter_name, description)|

It is false in our case and hence the NilClass on .fetch

I added the following:

    def template_default_parameters
      return {} unless TemplateUtils.template_hash(template)
      TemplateUtils.template_hash(template).fetch('Parameters', {}).inject({}) do |result, (parameter_name, description)|
        result[parameter_name] = description['Default']
        result
      end
    end

And that now passes that section but leads to another error:

Aws::CloudFormation::Errors::ValidationError Stack [mistack] does not exist

Maybe a we cannot make an update to a stack that is in REVIEW_IN_PROGRESS and we should abort if that is the case? This sounds all 🐔 🥚

[patrobinson]

It seems, according to the documentation {https://docs.aws.amazon.com/AWSCloudFormation/latest/APIReference/API_CreateChangeSet.html https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/using-cfn-describing-stacks.html#d0e11995), this state only exists for creating a change set from a Stack. Therefore 👍