-
Notifications
You must be signed in to change notification settings - Fork 5
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
Move the guts of the CLI into a class for easier testing #220
Conversation
# Create workflow/input pairs from the various combinations of paramaters | ||
workflows_inputs = | ||
if opts[:workflow_given] | ||
Optimist.die("cannot specify both --workflow and bare workflows") if args.any? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that this is in a class, these Optimist.die statements actually raise a SystemExit, which means this is kind of gross. I started refactoring to use an Optimist::Parser, but then this code started getting too complicated, so I left the PR as is to be a more pure moving of the code. We can follow up with better cleanup later if needed (or not - it's a CLI class after all).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think I added raise SystemExit
so we could test for this case (rather than using exit
)
4da92c9
to
629e6e0
Compare
Some comments on commit Fryguy@49daace lib/floe/cli.rb
|
Checked commit Fryguy@49daace with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint |
Fixed - ResultPath=$ replaces complete output (#199) - Fix retrier backoff values (#200) - Fix Retry issues (#202) - Add Apache-2.0 license (#217) Changed - Update gemspec summary (#205) - Simpler State#long_name (#204) - State only modifies Context#state - prep for Map/Parallel (#206) - Set StateHistory in Workflow not State (#211) - Make Runner#wait optional (#190) - Pass credentials around with context (#203) - Pass context to State without workflow (#216) - Move the guts of the CLI into a class for easy testing (#220) Added - Set State PreviousStateGuid in StateHistory (#208) - Add a codeclimate config file (#224) - Add an Execution unique ID to Context (#226)
I kept a singular test in there that ensures we are calling the exe/floe interface properly, but I'm really not sure if we need it or not. I'd be ok with dropping it. It would save an additional 0.13s knocking the time down to 0.85s or so
@kbrock Please review.
Fixes #218