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.

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

Bugfix for #23004 #23057

Conversation

@alexharv074
Copy link
Contributor

alexharv074 commented Oct 11, 2019

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".
The same comment appears in the analogous line in the Chef provisioner
and is useful.
@alexharv074

This comment has been minimized.

Copy link
Contributor Author

alexharv074 commented Oct 11, 2019

Copy link
Member

mildwonkey left a comment

I left one question in-line, but in general this LGTM!

builtin/provisioners/puppet/resource_provisioner.go Outdated Show resolved Hide resolved
@alexharv074 alexharv074 force-pushed the alexharv074:alexharvey/bugfix/23004_fix_default_ssh_type branch from 6f5077f to bbc213f Oct 11, 2019
@alexharv074 alexharv074 force-pushed the alexharv074:alexharvey/bugfix/23004_fix_default_ssh_type branch from e6881b0 to daf6329 Oct 11, 2019
@mildwonkey

This comment has been minimized.

Copy link
Member

mildwonkey commented Oct 11, 2019

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.

@alexharv074

This comment has been minimized.

Copy link
Contributor Author

alexharv074 commented Oct 12, 2019

@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.

alexharv074 added 2 commits Oct 11, 2019
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".
@alexharv074 alexharv074 force-pushed the alexharv074:alexharvey/bugfix/23004_fix_default_ssh_type branch from 6195594 to eb51f3c Oct 12, 2019
@alexharv074 alexharv074 requested a review from mildwonkey Oct 14, 2019
Copy link
Member

mildwonkey left a comment

I'll make sure this gets merged by the end of this week - thanks @alexharv074 ! I'm impressed with your follow-up 🎉

@mildwonkey mildwonkey merged commit cbeedfc into hashicorp:master Oct 17, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@mildwonkey

This comment has been minimized.

Copy link
Member

mildwonkey commented Oct 17, 2019

🎉

@hashibot

This comment has been minimized.

Copy link

hashibot bot commented Nov 17, 2019

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.

@hashibot hashibot bot locked and limited conversation to collaborators Nov 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants
You can’t perform that action at this time.