WinRb / winrm-elevated

Runs PowerShell commands as elevated over Windows Remote Management (WinRM) via a scheduled task
Apache License 2.0
27 stars 17 forks source link

Support for WinRM 1.5+ #2

Closed jerryk55 closed 8 years ago

jerryk55 commented 8 years ago

Allow winrm-elevated elevated runners to work alongside WinRM 1.5+ Executors. This allows one to run requests that don't require "elevation" in a single cmd process, while still running the elevated requests as winrm-elevated initially designed. We get the speed advantage of WinRM 1.5 with the authentication advantage of winrm-elevated.

sneal commented 8 years ago

Skipping the script upload should work even on older versions of WinRM unless an external process comes by and nukes the script. We never do any cleanup of the script once uploaded.

jerryk55 commented 8 years ago

True. I could have submitted two PRs, one to skip the upload and one to update to newer WinRM. Would you like me to do that?

Jerry

On Thu, Feb 25, 2016 at 5:41 PM, Shawn Neal notifications@github.com wrote:

Skipping the script upload should work even on older versions of WinRM unless an external process comes by and nukes the script. We never do any cleanup of the script once uploaded.

— Reply to this email directly or view it on GitHub https://github.com/WinRb/winrm-elevated/pull/2#issuecomment-189016396.

Jerry Keselman Red Hat Virtualization R&Djerryk@redhat.com | p. 212 530 7873

sneal commented 8 years ago

@jerryk55 Cool, if you don't mind just include code changes. I'd rather not lock down the the dependencies unless there's a real reason too. Thanks!

jerryk55 commented 8 years ago

@sneal without a change to the gemspec we can't use this gem. Its got winrm ~>1.3 and winrm-fs ~>0.2.2 - that doesn't let us use the new winrm features. This makes everybody choose to either use winrm-elevated or use the faster winrm executor (1.5 and later) features. Not both.

Jerry

On Fri, Feb 26, 2016 at 10:23 AM, Shawn Neal notifications@github.com wrote:

@jerryk55 https://github.com/jerryk55 Cool, if you don't mind just include code changes. I'd rather not lock down the the dependencies unless there's a real reason too. Thanks!

— Reply to this email directly or view it on GitHub https://github.com/WinRb/winrm-elevated/pull/2#issuecomment-189321405.

Jerry Keselman Red Hat Virtualization R&Djerryk@redhat.com | p. 212 530 7873

jerryk55 commented 8 years ago

The script upload has been broken out into a separate PR here: https://github.com/WinRb/winrm-elevated/pull/3. This change is required to allow winrm-elevated to work with winrm of at least version 1.5 and winrm-fs at least 0.2.0. We are not locking it down to those versions however.

@sneal If you can consider this for inclusion I would appreciate it, otherwise we will need to make a copy of this repository with our own winrm-elevated in order to run our code using both winrm-elevated and the 1.5 winrm. Which is obviously sub-optimal.

@mwrock maybe you could consider this as well.

Thanks so much!

mwrock commented 8 years ago

There does not appear to be anything in this gem that requires winrm > 1.3 nor does that constraint prevent the latest version from being loaded.

I think what @jerryk55 is getting at is that if you load this gem into a ruby environment that already has 1.3.x, it will not attempt to "upgrade" and that upgrade is desired in his scenario. Is that correct?

Typically you dont want to bump the gemspec versions unless you are taking a dependency on new functionality or are broken by an older version. If you simply want the new features to be available to your users, I'd explicitly gem install the latest version.

That should certainly work fine for winrm. However looking at the gem constraints, I'm not so sure about winrm-fs. Seems like 0.3.x will conflict. However looks like you are not concerned with 0.3. Even if we did decide this needed at least winrm 1.5, we'd want to make the constraint '~> 1.5' and not '>= 1.5' since the later would pull down 2.x when it releases which would surely break this gem as is.

jerryk55 commented 8 years ago

@mwrock Actually your suggested scenario was not what I was getting at, but that is because I misunderstood the gemspec file version operators. (I thought it went out to 3 digits even if you only specified 2). You are onto what my problem is when you start discussing winrm-fs.

The "winrm ~> 1.3" line in the current file is fine. That allows us to use winrm of 1.5 and higher (I upgraded to 1.7.2 yesterday). The line that causes a problem is the "winrm-fs ~> 0.2.2" line.
winrm-fs 0.2.2 has a dependency of winrm ~> 1.3.0 (not ~ 1.3 like winrm-elevated".

SO - to make a long story short - it looks like I have to either 1) PR winrm-elevated to allow winrm-fs ~0.2 (or ~0.3) instead of 0.2.2, OR 2) PR winrm-fs to allow winrm ~ 1.3 instead of 1.3.0.

I will make either change - whichever you think is better - I think the correct thing is to change winrm-fs instead of winrm-elevated.

mwrock commented 8 years ago

The latter is not an option since the latest winrm-fs corrected this with a constraint on 1.5. So to prevent "gem hell", I'm gonna recommend patching this gem to use:

s.add_runtime_dependency 'winrm', '~> 1.5'
s.add_runtime_dependency 'winrm-fs', '~> 0.3.0'
jerryk55 commented 8 years ago

@mwrock ok thanks. I will modify this PR and resubmit. I appreciate your help.

jerryk55 commented 8 years ago

@mwrock @sneal updated as per @mwrock's last suggestion and pushed.

sneal commented 8 years ago

Excellent, this LGTM :shipit: