-
Notifications
You must be signed in to change notification settings - Fork 9.7k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Bugfix for #23004 #23057
Bugfix for #23004 #23057
Conversation
alex-harvey-z3q
commented
Oct 11, 2019
The same comment appears in the analogous line in the Chef provisioner and is useful.
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.
I left one question in-line, but in general this LGTM!
6f5077f
to
bbc213f
Compare
e6881b0
to
daf6329
Compare
This looks great @alexharv074 ! I'm going to give @rodjek a little time to review this, but if we don't hear anything in a week or so (before the next release) I'll approve and merge. |
@mildwonkey further to my update yesterday, I found Tim's code and I saw how he was testing on Windows and set it all up and it's tested now on Windows. But the logic to provide feedback if password is absent didn't achieve anything. In fact, if password is empty, the Windows agent node hangs forever by the looks of it and never reaches my patch code--or maybe it eventually provides feedback after a 15 minute timeout. I wasn't patient enough to wait! Suffice to say I think error handling is needed somewhere to detect early that a user missed the password but it shouldn't be implemented in my bugfix. Whenever you and Tim are happy, I'm ok for you to merge this. |
Before this, the Terraform Puppet provisioner would error out in a confusing way if the type attribute in a connection block was not given. Apparently an omitted type leads to type having a value "" which must be then assumed to mean "ssh".
6195594
to
eb51f3c
Compare
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.
I'll make sure this gets merged by the end of this week - thanks @alexharv074 ! I'm impressed with your follow-up 🎉
🎉 |
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. |