Merging of winrm-fs and winrm-transporter

mwrock commented 8 years ago

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 commented 8 years ago

:+1:

smurawski commented 8 years ago

:+1: I'm all for limiting diverging paths for solving the same problems.

fnichol commented 8 years ago

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 :smile:

adamedx commented 8 years ago

:+1:

mwrock commented 8 years ago

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 https://github.com/WinRb/winrm-fs/issues/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 commented 8 years ago

cc @arlimus @chris-rock

chris-rock commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 https://github.com/WinRb/winrm-fs/pull/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:

Test against Test-Kitchen and Vagrant to ensure functional parity
Benchmark - Make sure this does not introduce perf regressions and provide some data as to how this compares with the current master.
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.
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 commented 8 years ago

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 https://github.com/WinRb/WinRM/pull/167
winrm-fs - new refctored file transfer logic https://github.com/WinRb/winrm-fs/pull/28
test-kitchen retrofitting - https://github.com/test-kitchen/test-kitchen/pull/902
train retrofitting - https://github.com/chef/train/pull/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 commented 8 years ago

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 commented 8 years ago

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

test-kitchen / winrm-transport