core: add WinRM support #1483
core: add WinRM support #1483
Conversation
|
@mitchellh @phinze I'm done for today, but would be nice if you can have a look at this one and let me know your thoughts about the code/approach so we can work towards getting this merged. And of course, enjoy the weekend! 😃 |
|
@mitchellh @phinze I know you guys are super busy, but I could really use some feedback on this one... I have a Chef provisioner lined up based on top of this PR, so I would like to get this one reviewed so I can either update or merge it... The PR is fully backwards compatible and all tests still work as expected, so I guess it's more a matter of the chosen solution and the actual code itself that is up for discussion/review. Thx... |
|
The implementation for this looks quite good. From a UX perspective, I wonder if the "communicator" abstraction is not correct for this. We have a bias towards SSH right now which is unfortunate... but I wonder if the correct approach instead is to introduce another provisioner type that is simply "winrm-file" "winrm-exec", etc. Then we can put all the connection info under that and we can talk about pulling that out into some sort of connection block later... I think that would just make the implementation a lot easier, and allow us to dedicate "file" and "remote-exec" specifically to SSH and the benefits it brings there. And vice versa WinRM can take full advantage of any benefits WinRM has. I only say this out of experience of trying to abstract "communicators" in Vagrant and Packer to mixed success and concern... |
|
@mitchellh thanks for the feedback! So if I understand correctly (not 100% sure about that) your OK with the way the communicators are abstracted, but you would prefer separate I must admit that I don't really see what type of benefits you a referring to when you mention "allow us to dedicate "file" and "remote-exec" specifically to SSH and the benefits it brings". Could you elaborate a little on that? Personally (coming from a 50/50 Linux/Windows shop) I really favour having just a single provisioner that knows how to copy a file or execute a remote command (regardless of the targeted OS) based on the given connection info. As for the communicator abstraction and looking at more higher level provisioners, my thoughts about having just a single provisioner are even stronger. Take for example the As for the implementation you are correct that it takes just a little more effort, but I believe that it pays off in usability and consistency for people using the provisioners. |
|
@svanharmelen You bring up very good points on all accounts. Alright, alright, I agree the current approach is best. I think this is on the right track then and LGTM. What else needs to be done? If not much I can do a much more thorough review, I only looked at general structure to start. |
|
@mitchellh cool! And yes, the next step would be to have the code thoroughly reviewed to see what needs to be done to get the PR up to spec... Lastly we need to have a small discussion on what tests we want/need for the functions in Of course those functions are almost empty shells around calls to the Looking at how the |
|
@mitchellh any progress on reviewing this one? Would be nice to get past the review so that I can fix any issues/comments and do the rebase in one go. Next to that I have some other pending features (to use a WinRM/SSH gateway for reaching servers is remote networks and the Chef provisioner) that I would like to start adding, but they depend on this one... |
|
@svanharmelen It's a rough POC at this point, but dylanmei/winrmtest is intended to help with the winrm testing in a similar way as net/http/httptest. It should be able to handle what you want. For covering more complicated workflows, such as large file uploads via packer-community/winrmcp, there's more work to be done. |
|
@dylanmei cool! Will have a look at the package and see if I can create some tests with it while waiting for @mitchellh to review this PR... Hope he or @phinze can make/find some time to review, so that I can do all remaining work (rebase, fix comments, add some tests) in one go... Thx! |
| @@ -382,7 +392,7 @@ func checkSCPStatus(r *bufio.Reader) error { | |||
| func scpUploadFile(dst string, src io.Reader, w io.Writer, r *bufio.Reader) error { | |||
| // Create a temporary file where we can copy the contents of the src | |||
| // so that we can determine the length, since SCP is length-prefixed. | |||
| tf, err := ioutil.TempFile("", "packer-upload") | |||
| tf, err := ioutil.TempFile("", "terraform-upload") | |||
|
Alright - completed a fairly thorough review on this. It's looking really, really solid! |
|
Cool... Will pick up the two remaining bits we just discussed and will also add some test before checking back in. Thx! |
|
Great - looking forward to it. I'll keep an eye out and we'll get this landed soon. 🚀 |
This is needed as preperation for adding WinRM support. There is still one error in the tests which needs another look, but other than that it seems like were now ready to start working on the WinRM part…
The reason why the shebang is removed from these tests, is because the shebang is only needed for SSH/Linux connections. So in the new setup the shebang line is added in the SSH communicator instead of in the resource provisioner itself…
* We now return an error when you set the script_path to C:\Windows\Temp explaining this is currently not supported * The fix in PR #1588 is converted to the updated setup in this PR including the unit tests Last thing to do is add a few tests for the WinRM communicator…
Pretty nice test coverage this way, covering all WinRM actions/logic by using the winrmtest package. I had to extend/update/fix that package a little here and there, but it now serves a nice purpose for testing WinRM stuff…
The winrmtest package update is merged now, so this can be merged now as well…
|
Most likely failed because of |
This will likely be a temp fix until `make updated eps` is made a little smarter by @phinze 😉
|
Alright looks good! |
|
@phinze thanks for taking some time to review and get this one in! |
|
Oh snap. Thanks for the PR and thanks for the merge. I want to play with nano server and WinRM enables that. 💯 |
|
Just another batch of amazing work from @svanharmelen 😀 |
|
👍 |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Fully working and tested solution. Did the proper refactoring of all existing providers and updated the docs as well. All existing tests are still OK, but some needed to be tweaked for the new setup...
Only thing left seems to be adding some tests for the winrm communicator functions.
/CC #1201