-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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 remote_exec on Windows SSH #26865
Conversation
Codecov Report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! I appreciate you moving this forward.
Some quick notes:
- The use of a global
isWindows
variable doesn't feel right to me. It's shared mutable state between (potentially) multiple communicators. If it's necessary state, this property should be isolated to a single communicator instance. - I'm also uncertain that this is the right approach. To me, it feels a bit too implicit to sniff a path format and require a drive letter to detect a Windows SSH host. Instead we might want a new property on the
connection
configuration, for exampletarget_platform
, with valid values beingunix
andwindows
. What do you think about that alternative? - No matter what approach we take, we'll need to update documentation, both on the provisioner connection configuration and the remote exec provisioner pages.
Hi alisdair, I would more than happy to try change this PR to add the target_platform property on the connection configuration, if you can point me in the right direction about where to look. |
I've been browsing through the code a bit and have changed the config to add a target_platform property. |
Also updated the provisioner connection configuration docs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, @hhofs! 🙌
I tested these changes against AWS Windows and Ubuntu EC2 instances, and everything works as expected. I've left a few very minor suggestions inline, but I think this is otherwise ready to merge.
Thanks again for your work on this, it's much appreciated!
Co-authored-by: Alisdair McDiarmid <[email protected]>
Hi @alisdair , thx for the thorough review. I've incorporated your requested changes in my latest commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks again! 🎉
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. |
EDIT: changed solution to use a target_platform property in the connection block
Summary
A small fix for #25634 where certain parts of the remote_exec (chmodding, inserting a shebang requesting a pty) are skipped when the target file is a windows path. This is determined by looking at the first 2 runes of the destination path, if the first rune in a range [a-z]:[A-Z] and the second rune is a colon the isWindows var is set to true.
CMD configuration
Output:
PS1 Configuration
Output