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

Support for WinRM 1.5+ #2

Merged
merged 1 commit into from
Mar 2, 2016
Merged

Conversation

jerryk55
Copy link
Contributor

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
Copy link
Member

sneal commented Feb 25, 2016

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
Copy link
Contributor Author

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
#2 (comment).

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

@sneal
Copy link
Member

sneal commented Feb 26, 2016

@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
Copy link
Contributor Author

@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
#2 (comment).

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

@jerryk55 jerryk55 force-pushed the add_winrm_1.7_support branch from 349a443 to 04ba8bf Compare March 1, 2016 18:16
@jerryk55
Copy link
Contributor Author

jerryk55 commented Mar 1, 2016

The script upload has been broken out into a separate PR here: #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
Copy link
Member

mwrock commented Mar 2, 2016

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
Copy link
Contributor Author

jerryk55 commented Mar 2, 2016

@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
Copy link
Member

mwrock commented Mar 2, 2016

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
Copy link
Contributor Author

jerryk55 commented Mar 2, 2016

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

Allow winrm-elevated elevated runners to work alongside WinRM 1.5+
Executors and winrm-fs at least 0.3.0
@jerryk55 jerryk55 force-pushed the add_winrm_1.7_support branch from 04ba8bf to 44930ef Compare March 2, 2016 16:57
@jerryk55
Copy link
Contributor Author

jerryk55 commented Mar 2, 2016

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

@sneal
Copy link
Member

sneal commented Mar 2, 2016

Excellent, this LGTM :shipit:

sneal added a commit that referenced this pull request Mar 2, 2016
@sneal sneal merged commit 7734ebb into WinRb:master Mar 2, 2016
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.

3 participants