Skip to content
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

adding command_executor to support multiple commands from a single shell #167

Merged
merged 1 commit into from
Jan 10, 2016

Conversation

mwrock
Copy link
Member

@mwrock mwrock commented Dec 26, 2015

Deprecates WinRM::WinRMWebService methods cmd, run_cmd, powershell, and run_powershell_script in favor of the run_cmd and run_powershell_script methods of the WinRM::CommandExecutor class. The CommandExecutor allows multiple commands to be run from the same WinRM shell providing a significant performance improvement whwn issuing multiple calls.

This PR is a prerequisite for WinRb/winrm-fs#28 which depend on CommandExecutor, which is being added here since it provides value beyond just transferring files.

@mwrock
Copy link
Member Author

mwrock commented Dec 26, 2015

Note that I have removed the ShellCloser class used as a finalizer for CommandExecutor. While users of CommandExecutor are encouraged to call close explicitly, I think we can leverage the finalizer in a transparent manner upon GC.

I'd be curious to know if @fnichol has any words of caution regarding this strategy. I have tested GC's on the CommandExecutor as it is here and have not had issues with it losing sessions.

@mwrock
Copy link
Member Author

mwrock commented Jan 1, 2016

I just made a few additional changes here:

  • Moved the retryable method from the fs-transport winrm-fs branch into the CommandExecutor. Made more sense for that to live here and its a nice feature to have for openning shells.
  • Expose the WinRMWebService logger via a attr_reader and set its level to :warn and add the stdout appender on initialization. WinRMWebService consumers can access its logger and change level, appenders, etc. This will be helpful when it is plugged into Test-Kitchen and make it more natural to integrte the winrm logging into the test-kitchen log streams.
  • Added a convenience method to WinRMWebService: create_executor. This can be used 2 ways:
winrm.create_executor do |executor|
  executor.run_cmd('ipconfig')
end

This ensures the executor is closed when done.

executor = winrm.create_executor
executor.run_cmd('ipconfig')
executor.close

@mwrock
Copy link
Member Author

mwrock commented Jan 1, 2016

I think this is ready to review. @sneal let me know if you think there is more to change or if I'm off anywhere.

@sneal
Copy link
Member

sneal commented Jan 1, 2016

@mwrock Awesome stuff here, I love seeing things getting pushed upstream for reusability. I'll take a good look later next week when I have my laptop.

### Deprecated methods
As of version 1.5.0 `WinRM::WinRMWebService` methods `cmd`, `run_cmd`, `powershell`, and `run_powershell_script` have been deprecated and will be removed from the next major version of the WinRM gem.

Use the `run_cmd` and `run_powershell_script` of the `WinRM::CommandExecutor` class instead. The `CommandExecutor` allows multiple commands to be run from the same WinRM shell providing a significant performance improvement whwn issuing multiple calls.
Copy link
Member

Choose a reason for hiding this comment

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

whwn => when

@sneal
Copy link
Member

sneal commented Jan 6, 2016

:shipit:

@mwrock mwrock force-pushed the executor branch 2 times, most recently from 00aca31 to 86ef08a Compare January 10, 2016 04:11
@mwrock
Copy link
Member Author

mwrock commented Jan 10, 2016

rebase/squashed and removing 1.5 versioning artifacts. Will merge that in a separate PR

mwrock added a commit that referenced this pull request Jan 10, 2016
adding command_executor to support multiple commands from a single shell
@mwrock mwrock merged commit 55ba0b9 into master Jan 10, 2016
@mwrock mwrock deleted the executor branch January 10, 2016 04:35
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.

2 participants