Handle exit status from the CLI class #310
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
orien
commented
Mar 7, 2020
| @@ -10,8 +10,7 @@ end | |||
| trap("SIGINT") { raise StackMaster::CtrlC } | |||
|
|
|||
| begin | |||
| result = StackMaster::CLI.new(ARGV.dup).execute! | |||
| exit !!result | |||
| StackMaster::CLI.new(ARGV.dup).execute! | |||
There was a problem hiding this comment.
This binstub no longer handles the exit status.
orien
commented
Mar 7, 2020
| @@ -253,7 +253,7 @@ def execute_stacks_command(command, args, options) | |||
| end | |||
| end | |||
| end | |||
| success | |||
| @kernel.exit false unless success | |||
There was a problem hiding this comment.
The CLI class now handles the exit status.
orien
commented
Mar 7, 2020
| Feature: Check the StackMaster version | ||
| Scenario: Use the --version option | ||
| When I run `stack_master --version` | ||
| Then the exit status should be 0 |
There was a problem hiding this comment.
Add a scenario to ensure stack_master --version results in a 0 status.
orien
commented
Mar 7, 2020
| @@ -123,6 +123,7 @@ Feature: Apply command | |||
| | + "Vpc": { | | |||
| | Parameters diff: | | |||
| | KeyName: my-key | | |||
| And the exit status should be 0 | |||
There was a problem hiding this comment.
Validate the exit status in all Cucumber scenarios.
orien
commented
Mar 7, 2020
| @@ -43,7 +43,7 @@ Feature: Apply command with allowed accounts | |||
| And I run `stack_master apply us-east-1 myapp-db` | |||
| And the output should contain all of these lines: | |||
| | Account '11111111' is not an allowed account. Allowed accounts are ["22222222"].| | |||
| Then the exit status should be 0 | |||
| Then the exit status should be 1 | |||
There was a problem hiding this comment.
This is an example of an incorrect Cucumber validation. This scenario was ensuring a status 0, but because it was only testing via the CLI class it missed the binstub returning 1.
Now the feature correctly checks the value returned in the command invocation.
Note: this is not a change in StackMaster behaviour.
patrobinson
approved these changes
Mar 9, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
There are a couple of peculiar StackMaster behaviours regarding exit status.
stack_master --versionresults in a status of1(stack_master --version returns a failed exit code #183).stack_masterbinstub, but the Cucumber feature suite doesn't test this. It drives StackMaster via theStackMaster::CLIclass directly. This makes validating the status code impossible.Change
I propose we move handling of the exit status into the
StackMaster::CLIclass. This will solve both problems:stack_master --versionwill return a status of0(fixes stack_master --version returns a failed exit code #183).