Merging of winrm-fs and winrm-transporter

[mwrock]

The winrm-fs gem and this gem are nearly identical in both function and public interface. I'd like to propose that we merge this gem over to winrm-fs to consolidate reuse. Given that the scope of these gems is small and their implementation are both very close, I think this can be done quickly and easily - I am volunteering. Both are actually based on my original winrm file transfer code but each polished off by smarter folk.

This gem has some nice functionality that handles limited batched commands on win7/2k8 and the other has a couple niceties like ensuring all powershell silences the progress stream which has bitten us here.

I'm nominating winrm-fs as the "official" home simply because it has a more generic audience in the winrm org and this one is aimed more towards test-kitchen consumption (not that it has to).

Any thoughts or objections?

cc @sneal

[sneal]

👍

[smurawski]

👍 I'm all for limiting diverging paths for solving the same problems.

[fnichol]

That sounds great to me as well. I was basing a lot of API and design off winrm-fs but due to timing and our streaming-files-of-unknown-size requirements put a lot of the work here. A side goal of this code (once we extracted it from test-kitchen proper) was to augment or wrap existing winrm objects. I was also trying to steer clear of native code in the dependency chain, so if that's a similar goal or nice-to-have in winrm-fs then we're in good shape. I also suspect we're going to use this elsewhere in Chef ecosystem down the road so I love more downstream code using this all day long 😄

[adamedx]

👍

[mwrock]

I went through both gems today and have these notes:

winrm-Transport

Used by

• Test-Kitchen
• Train

Entry points

• CommandExecutor
• FileTransporter

Notes

• CommandExecutor provides support for limited command count of legacy windows versions
• CommandExecutor protects against dangling connections with a finalizer
• Overall a more efficient implementation that can upload multiple directories and files in a single call that streams everything over a single connection.

winrm-fs

Used by

• vagrant
• others?

Entry points

• FileManager

Notes

• Contains a CommandExecutor with the same public interface as the one in winrm-transport.
• Provides safety against the progress stream.
• Supports streamed output for implementing progress bars
• Supports downloads and other basic file/directory operations

general comments

• CommandExecutor has appeal beyond file transfers because of the convenience to run several commands on the same connection, but it is only intended to be publicly consumed in winrm-transport

Ideal home for CommandExecutor

Move CommandExecutor into WinRM proper and allow users to use it over the current helper service single call methods. Discussion started at WinRb/winrm-fs#22

Not so ideal options:

• keep winrm-transport CommandExecutor for use by test-kitchen ad hoc commands (two implementations)
• Test-kitchen ad hoc commands call command executor in winrm-fs (weird using a file transport gem for commands)
• Test-Kitchen winrm service directly for ad hoc commands (lose single connection perf gain)

[mwrock]

cc @arlimus @chris-rock

[chris-rock]

I really like to see the projects merged. It makes sense for me to move most winrm parts to the official winrm. I'd like to ensure that we:

• only move general winrm code to the winrm project
• keep specific wrapper code, that is essential for test-kitchen or train in the related projects

[sneal]

@mwrock Moving CommandExecutor into core winrm makes sense to me. I dislike adding yet another way to do things to the core library, but until we decide to completely redesign the API for v2 I think this is a good iterative approach.

[mwrock]

I did not get far at all on this in November but have found time the last couple nights to return and have a "working" pr at WinRb/winrm-fs#28. "working" as in all the winrm-fs integration tests pass with the refactored merge.

Here is the todo list copied from that PR as I see it:

1. Test against Test-Kitchen and Vagrant to ensure functional parity
2. Benchmark - Make sure this does not introduce perf regressions and provide some data as to how this compares with the current master.
3. Possibly bring back the zip file factories. For simplicity, I stripped the shell implementation. The benchmarking (mentioned above) should prove if its worth adding back.
4. Move the CommandExecutor and ShellCloser into the WinRM gem and possibly refactor how ShellCloser is interacted with.

I'll move discussion to the thread of that PR until things are ready to be deprecated here.

[mwrock]

I think the work here is pretty well wrapped up and ready for folks to review if interested. Here are the relevant PRs:

• winrm - CommandExecutor and retry logic adding command_executor to support multiple commands from a single shell WinRb/WinRM#167
• winrm-fs - new refctored file transfer logic Consolidation of winrm-transport into winrm-fs WinRb/winrm-fs#28
• test-kitchen retrofitting - Deprecating winrm-tansport and winrm-s gems test-kitchen#902
• train retrofitting - Deprecating winrm-tansport gem inspec/train#46

There was a small but breaking change in winrm-fs when uploading directories to comply with the strategy used in winrm-transport that includes the creation of the base directory being uploaded. I was concerned tha tthis would require fixes in vagrant but it looks like vagrant only uses upload for individual files.

[mwrock]

The merged winrm-fs is now released as winrm-fs v0.3.0. Just a matter of polishing train and Test-kitchen PRs now.

[mwrock]

closing - winrm-fs has been merged into train and test kitchen. Will adjust the readme tonight to signify this is a deprecated gem.