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.

Sign up for GitHub

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

Jump to bottom

Expose additional winrm options #392

Merged
merged 3 commits into from
Dec 19, 2018
Merged

Expose additional winrm options #392

merged 3 commits into from
Dec 19, 2018

Conversation

frezbo
Copy link
Contributor

@frezbo frezbo commented Dec 12, 2018

Signed-off-by: Noel Georgi git@frezbo.com

Fixes: #391

@frezbo
Expose additional winrm options
6a1660e 
Signed-off-by: Noel Georgi <git@frezbo.com>
@frezbo frezbo force-pushed the feature/expose-missing-winrm-opts branch from e7479ab to 6a1660e Compare December 12, 2018 20:17
frezbo added a commit to frezbo/inspec that referenced this pull request Dec 12, 2018
@frezbo
Expose additonal WInRM opts are per inspec/train#392
9230fb0 
Signed-off-by: Noel Georgi <git@frezbo.com>
@frezbo
Copy link
Contributor Author

frezbo commented Dec 12, 2018

rerunning to fix the GCP fluke.

@frezbo frezbo closed this Dec 12, 2018
@frezbo frezbo reopened this Dec 12, 2018
@frezbo
Copy link
Contributor Author

frezbo commented Dec 12, 2018

The failure is related to GCP test suite.

@frezbo frezbo mentioned this pull request Dec 18, 2018
Merged
clintoncwolfe
clintoncwolfe requested changes Dec 18, 2018
View reviewed changes
Copy link
Contributor

@clintoncwolfe clintoncwolfe left a comment

Choose a reason for hiding this comment

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

Thanks so much for this! Main request is to add prefix as disussed; and a desire for test coverage, which is optional here.

lib/train/transports/winrm.rb Outdated
@@ -45,6 +45,9 @@ class WinRM < Train.plugin(1)
option :port
option :user, default: 'administrator', required: true
option :password, nil
option :transport, default: :negotiate
Copy link
Contributor

Choose a reason for hiding this comment

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

For now, let's add a --winrm- prefix here, to match what InSpec will be sending. I think in the future we may be able to something like having InSpec strip the prefix from options (or all options would have the prefix - point being it should be consistent)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved.

lib/train/transports/winrm.rb Outdated
transport: :negotiate,
disable_sspi: false,
basic_auth_only: false,
transport: opts[:transport].intern,
Copy link
Contributor

Choose a reason for hiding this comment

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

It's equivalent, but more readable, to say .to_sym here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, come to think of it, we might want to add some validation here; I'm not sure what valid values are for the transport flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved.

lib/train/transports/winrm.rb Outdated
@@ -45,6 +45,9 @@ class WinRM < Train.plugin(1)
option :port
option :user, default: 'administrator', required: true
option :password, nil
option :transport, default: :negotiate
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great if you could add a test file in test/windows/ to exercise the new options (those tests are run by rake test:windows, see Rakefile for details). If that would be overly difficult to do, I'm OK with omitting that for now - but please open a new issue on Train mentioning the need to add testing for the new winrm options, so it doesn't get lost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved.

@frezbo
Add UT's for winrm transport
7336a8c 
Signed-off-by: Noel Georgi <git@frezbo.com>
@frezbo
Copy link
Contributor Author

frezbo commented Dec 19, 2018

Closing and re-running to rule out the Cloud Provider UT's, they are a fluke on local workstation too.

@frezbo frezbo closed this Dec 19, 2018
@frezbo frezbo reopened this Dec 19, 2018
@frezbo
Copy link
Contributor Author

frezbo commented Dec 19, 2018

@clintoncwolfe I have updated the PR as per the review. The tests are failing due to gcp UT's failing. (It is also faky on the local machine, would be good to fix that). I have omitted kerberos transport. The ssl transport also exposes some additional options, but it would go out of scope of this MVP. I think it would be good to track that later. I have also added a baseline UT's for the winrm transport.

frezbo added a commit to frezbo/inspec that referenced this pull request Dec 19, 2018
@frezbo
Expose additonal WInRM opts are per inspec/train#392
d6b0af8 
Signed-off-by: Noel Georgi <git@frezbo.com>
@frezbo frezbo closed this Dec 19, 2018
@frezbo frezbo reopened this Dec 19, 2018
@frezbo
Copy link
Contributor Author

frezbo commented Dec 19, 2018

Re-trigger tests

@frezbo
Making rubocop happy
907539b 
Signed-off-by: Noel Georgi <git@frezbo.com>
@clintoncwolfe clintoncwolfe mentioned this pull request Dec 19, 2018
Closed
jquick
jquick approved these changes Dec 19, 2018
View reviewed changes
Copy link
Contributor

@jquick jquick left a comment

Choose a reason for hiding this comment

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

Thanks @frezbo !

@clintoncwolfe clintoncwolfe merged commit 30043a5 into inspec:master Dec 19, 2018
@clintoncwolfe
Copy link
Contributor

Disregarding unrelated failing unit test per #393

@frezbo frezbo deleted the feature/expose-missing-winrm-opts branch December 20, 2018 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jquick jquick jquick approved these changes

@clintoncwolfe clintoncwolfe clintoncwolfe approved these changes

Assignees
No one assigned
Labels
Expeditor: Bump Minor Version Platform: WinRM Type: New Feature Adds new functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expose additional winrm options
4 participants
@frezbo @clintoncwolfe @jquick @zenspider