Skip to content

no need for explicit exit since bin/stack_master already handles that#280

Merged
patrobinson merged 2 commits intoenvato:masterfrom
grosser:grosser/exit
Mar 20, 2019
Merged

no need for explicit exit since bin/stack_master already handles that#280
patrobinson merged 2 commits intoenvato:masterfrom
grosser:grosser/exit

Conversation

@grosser
Copy link
Copy Markdown
Contributor

@grosser grosser commented Mar 19, 2019

followup to #276 which fixed 1 bug and introduced another 😨
the cucumber test was misleading since it never executes bin/stack_master and so never gets the actual exit status
also simplifying by converting the array logic to a boolean

replaces #279
fixes #277
fixes #278

Copy link
Copy Markdown
Contributor

@patrobinson patrobinson left a comment

Choose a reason for hiding this comment

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

I want to add some integration tests for validate as well since that's been having issues

Comment thread features/apply.feature Outdated
When I run `stack_master apply foo bar`
Then the output should contain "Could not find stack definition bar in region foo"
And the exit status should be 1
And the exit status should be 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wouldn't this be 1 based on if stack_definitions.empty? ... success = false ?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

it would be 1 if success returns false, because exit !!false in bin/stack_master.rb

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it will never be 1 since we do not run bin/stack_master in this test, which includes the exit

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should just remove this test rather than introduce something that tests for undesirable behaviour.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

then it should be removed from all of them for consistency 🤷‍♂️

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While the other tests are not accurate they do reflect the behaviour we want to see.

This test is both inaccurate and does not reflect true behaviour

@grosser
Copy link
Copy Markdown
Contributor Author

grosser commented Mar 19, 2019

tests for validate can be their own PR, would prefer to get this out to get the bug fixed

Copy link
Copy Markdown

@logingood logingood left a comment

Choose a reason for hiding this comment

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

👍
can we get it merged sooner, otherwise our jenkins builds are broken :(

Comment thread features/apply.feature Outdated
Co-Authored-By: grosser <michael@grosser.it>
@grosser
Copy link
Copy Markdown
Contributor Author

grosser commented Mar 20, 2019

updated

@patrobinson patrobinson merged commit 93d5301 into envato:master Mar 20, 2019
@patrobinson
Copy link
Copy Markdown
Contributor

Released as 1.13.1

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.

stack_master validate exits with non-zero on valid stack stack_master apply with no changes exits with non-zero

3 participants