Allow --nodes to be declared multiple times #103
Conversation
cyberious
commented
Oct 12, 2017
cyberious
commented
- Allow for the user to declare --nodes multiple times along a cli and merge them into a single array, previously it would allow for last nodes declaration to win and miss any previous calls on cli
|
Resolves #100 |
lib/bolt/cli.rb
Outdated
| @@ -109,7 +109,8 @@ def create_option_parser(results) | |||
| '* protocol is `ssh` by default, may be `ssh` or `winrm`', | |||
| '* port is `22` by default for SSH, `5985` for winrm (Optional)' | |||
| ) do |nodes| | |||
| results[:nodes] = parse_nodes(nodes) | |||
| results[:nodes] = [] unless results[:nodes] | |||
| results[:nodes] = results[:nodes] + parse_nodes(nodes) | |||
There was a problem hiding this comment.
This could cause a regression on uniqness as implemented in #96.
A more idiomatic ruby would be to:
results[:nodes] ||= []
results[:nodes] += parse_nodes(nodes)
There was a problem hiding this comment.
The problem would be you are assigning the result a nil list if not parsed again, so if it fails the last parse it will than give an empty list?
There was a problem hiding this comment.
Also at the point of parsing the results[:nodes] would be nil
There was a problem hiding this comment.
Also now that I am looking more, any reason not to switch to a struct style options parser pattern?
|
modified to have the options default to empty node list and validate that it is indeed not empty or fail accordingly |
|
I will get back to fix my whitespace issues from my IDE but syntactic review and will goback on other |
lib/bolt/cli.rb
Outdated
| parsed = parse_nodes(nodes) | ||
| if parsed | ||
| results[:nodes] += parsed | ||
| end |
There was a problem hiding this comment.
If we pass --nodes a --nodes a then we'll get duplicates, which would regress on 555b4ed. Also parse_nodes always returns an array (possible empty), so checking if parsed isn't needed.
There was a problem hiding this comment.
correct, but we are ensuring Uniq in a secondary PR that would do away with that is a separate issue can and can exist even without the secondary nodes flag.
|
Thanks @cyberious! Can you squash your commits and resolve conflicts? |
|
Squashed and rebased |
spec/bolt/cli_spec.rb
Outdated
| @@ -73,6 +73,11 @@ | |||
| expect(cli.parse).to include(nodes: %w[foo bar]) | |||
| end | |||
|
|
|||
| it "accepts multiple nodes across multiple declarations" do | |||
| cli = Bolt::CLI.new(%w[command run --nodes foo,bar --nodes more,bars]) | |||
| expect(cli.parse).to include(nodes: %w[foo bar more bars]) | |||
There was a problem hiding this comment.
Can you add the same node name to both --nodes flags to verify the uniqueness is working?
| @@ -73,6 +73,11 @@ | |||
| expect(cli.parse).to include(nodes: %w[foo bar]) | |||
| end | |||
|
|
|||
| it "accepts multiple nodes across multiple declarations" do | |||
| cli = Bolt::CLI.new(%w[command run --nodes foo,bar --nodes bar,more,bars]) | |||
There was a problem hiding this comment.
@joshcooper just added bar in second location to ensure uniq still works
|
Thanks @cyberious. One minor nit, it looks like your commit message has your work email address, but our CLA system only knows about your |
- Allow for the user to declare --nodes multiple times along a cli and merge them into a single array, previously it would allow for last nodes declaration to win and miss any previous calls on cli - Ensure we are still having a uniq list after parsing nodes each pass - As we now take nodes multiple times we need to ensure we are still getting a unique list after we have appended to the original list of items, previously this was on a single pass of nodes
|
Updated to my personal |
|
CLA signed by all contributors. |
|
Had to take a double check, showed closed and not merged... not sure what is up with github on this one. |
|
Thanks @cyberious! I updated the commit message to reference the JIRA ticket and merged in 4b2f043 |
