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

Merge knife-windows into chef #9354

Closed
wants to merge 58 commits into from
Closed

Merge knife-windows into chef #9354

wants to merge 58 commits into from

Conversation

tas50
Copy link
Contributor

@tas50 tas50 commented Feb 7, 2020

Ship the knife-windows plugin directly in Chef.

Signed-off-by: Tim Smith tsmith@chef.io

@tas50 tas50 requested review from a team as code owners February 7, 2020 02:38
@tas50 tas50 force-pushed the knife_windows branch 2 times, most recently from c7cd96b to 7df879b Compare February 13, 2020 20:17
tas50
tas50 commented Feb 13, 2020
View reviewed changes
lib/chef/knife/windows_cert_install.rb Outdated

begin
ui.info "Adding certificate to the Windows Certificate Store..."
result = `powershell.exe -Command " '#{config[:cert_passphrase]}' | certutil -importPFX '#{file_path}' AT_KEYEXCHANGE"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this something we want to rewrite to use the win32-cert gem instead or is shelling out aceptable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the least we should use a proper shellout

tas50
tas50 commented Feb 13, 2020
View reviewed changes
lib/chef/knife/windows_listener_create.rb Outdated
begin
if config[:cert_install]
config[:cert_passphrase] = get_cert_passphrase unless config[:cert_passphrase]
result = `powershell.exe -Command " '#{config[:cert_passphrase]}' | certutil -importPFX '#{config[:cert_install]}' AT_KEYEXCHANGE"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same for this one. Should we do this via the win32-cert or simply refactor it to use a shellout

tas50
tas50 commented Feb 13, 2020
View reviewed changes
lib/chef/knife/winrm.rb Outdated

require_relative "../knife"
require_relative "winrm_knife_base" # WinrmCommandSharedFunctions
require_relative "winrm_session"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These need to get moved into the deps block to match the work I did in knife-windows

@tas50 tas50 added the Status: Waiting on Contributor A pull request that has unresolved requested actions from the author. label Feb 24, 2020
@tas50 tas50 changed the title WIP: Merge knife-windows into chef Merge knife-windows into chef Feb 26, 2020
@tas50 tas50 force-pushed the knife_windows branch 3 times, most recently from b3b01aa to 43951b9 Compare February 28, 2020 05:07
@tas50 tas50 self-assigned this Mar 3, 2020
@tas50 tas50 requested a review from a team as a code owner June 26, 2020 15:56
phiggins
phiggins reviewed Jun 26, 2020
View reviewed changes
lib/chef/knife/winrm.rb Outdated Show resolved Hide resolved
phiggins
phiggins approved these changes Jun 26, 2020
View reviewed changes
Copy link
Contributor

@phiggins phiggins left a comment

Choose a reason for hiding this comment

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

There is a lot here and I didn't look over all of it very closely.

tas50 and others added 26 commits September 25, 2020 18:35
@tas50 @phiggins
Update lib/chef/knife/winrm_knife_base.rb
da24159 
Signed-off-by: Tim Smith <tsmith@chef.io>

Co-authored-by: pete higgins <pete@peterhiggins.org>
@tas50 @phiggins
Update lib/chef/knife/winrm_knife_base.rb
95275a2 
Signed-off-by: Tim Smith <tsmith@chef.io>

Co-authored-by: pete higgins <pete@peterhiggins.org>
@tas50 @phiggins
Update lib/chef/knife/winrm_knife_base.rb
0679244 
Signed-off-by: Tim Smith <tsmith@chef.io>

Co-authored-by: pete higgins <pete@peterhiggins.org>
@tas50 @phiggins
Update lib/chef/knife/winrm_knife_base.rb
3820fee 
Signed-off-by: Tim Smith <tsmith@chef.io>

Co-authored-by: pete higgins <pete@peterhiggins.org>
@tas50
Remove odd require on other plugins within the winrm plugin
6c63310 
Signed-off-by: Tim Smith <tsmith@chef.io>
@tas50
Rename confusing config check method
66e3a6b 
'
Signed-off-by: Tim Smith <tsmith@chef.io>
@tas50
Rename more methods
e773aa5 
Signed-off-by: Tim Smith <tsmith@chef.io>
@tas50
Nuke a method
d24d1d9 
Signed-off-by: Tim Smith <tsmith@chef.io>
@tas50 @phiggins
Update lib/chef/knife/winrm_session.rb
e1fb23e 
Signed-off-by: Tim Smith <tsmith@chef.io>

Co-authored-by: pete higgins <pete@peterhiggins.org>
@tas50
Simplify code / avoid mutating config
2e1ba1c 
Signed-off-by: Tim Smith <tsmith@chef.io>
@tas50
Simplify fetching correct winrm user
4a29e9a 
Signed-off-by: Tim Smith <tsmith@chef.io>
@tas50
Rename methods to remove resolve_
6f17d71 
Signed-off-by: Tim Smith <tsmith@chef.io>
@tas50 @phiggins
Update spec/unit/knife/windows_cert_install_spec.rb
d7ca400 
Signed-off-by: Tim Smith <tsmith@chef.io>

Co-authored-by: pete higgins <pete@peterhiggins.org>
@tas50 @phiggins
Update spec/unit/knife/windows_cert_install_spec.rb
2eb7e83 
Signed-off-by: Tim Smith <tsmith@chef.io>

Co-authored-by: pete higgins <pete@peterhiggins.org>
@tas50 @phiggins
Update lib/chef/knife/windows_cert_generate.rb
8ad14c3 
Signed-off-by: Tim Smith <tsmith@chef.io>

Co-authored-by: pete higgins <pete@peterhiggins.org>
@tas50 @phiggins
Update lib/chef/knife/wsman_test.rb
6bb5d6f 
Signed-off-by: Tim Smith <tsmith@chef.io>

Co-authored-by: pete higgins <pete@peterhiggins.org>
@tas50 @phiggins
Update lib/chef/knife/wsman_test.rb
1a8d2bc 
Signed-off-by: Tim Smith <tsmith@chef.io>

Co-authored-by: pete higgins <pete@peterhiggins.org>
@tas50 @phiggins
Update lib/chef/knife/winrm_shared_options.rb
7550705 
Signed-off-by: Tim Smith <tsmith@chef.io>

Co-authored-by: pete higgins <pete@peterhiggins.org>
@tas50 @phiggins
Update spec/unit/knife/windows_cert_generate_spec.rb
20674d7 
Signed-off-by: Tim Smith <tsmith@chef.io>

Co-authored-by: pete higgins <pete@peterhiggins.org>
@tas50 @phiggins
Update spec/unit/knife/winrm_session_spec.rb
f3ae9d2 
Signed-off-by: Tim Smith <tsmith@chef.io>

Co-authored-by: pete higgins <pete@peterhiggins.org>
@tas50 @phiggins
Update spec/unit/knife/winrm_spec.rb
f33f841 
Signed-off-by: Tim Smith <tsmith@chef.io>

Co-authored-by: pete higgins <pete@peterhiggins.org>
@tas50 @phiggins
Update spec/unit/knife/wsman_test_spec.rb
8c6b93e 
Signed-off-by: Tim Smith <tsmith@chef.io>

Co-authored-by: pete higgins <pete@peterhiggins.org>
@tas50
Remove duplicate support file
e75d9d9 
Signed-off-by: Tim Smith <tsmith@chef.io>
@tas50
Fix failing specs
34d7a7c 
Signed-off-by: Tim Smith <tsmith@chef.io>
@tas50
Fix cspell and chefstyle errors
9e6a6f1 
Signed-off-by: Tim Smith <tsmith@chef.io>
@tas50
Chefstyle fix
a4b1675 
Signed-off-by: Tim Smith <tsmith@chef.io>
@tas50 tas50 closed this Feb 13, 2021
@tas50
Copy link
Contributor Author

tas50 commented Feb 13, 2021

Closing this so we can do it the correct way post #9354

@tas50 tas50 deleted the knife_windows branch April 2, 2021 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lamont-granquist lamont-granquist lamont-granquist left review comments

@phiggins phiggins phiggins approved these changes

Assignees

@tas50 tas50

Labels
Platform: Windows Status: Waiting on Contributor A pull request that has unresolved requested actions from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tas50 @kvivek1115 @phiggins @lamont-granquist