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

[JENKINS-57967] Pass SshHostKeyVerificationStrategy to SSHLauncher #104

Merged
merged 3 commits into from
Jul 8, 2019

Conversation

medianick
Copy link
Contributor

@medianick medianick commented Jun 26, 2019

Previous version of the constructor was removed in SSH Slaves 1.30.0. Fixes problem detailed in https://issues.jenkins-ci.org/browse/JENKINS-57967.

First version where SshHostKeyVerificationStrategy was added to the SSHLauncher ctor
@pjdarton
Copy link
Member

Removed in 1.30.0? Ah, I guess I best not update that plugin on my vSphere-using Jenkins servers then...

Once you're happy that this PR is complete, let me know; we can then both test the results in our respective "real world" environments and, if that all looks good, I'll kick out a new release...

@medianick
Copy link
Contributor Author

I believe this PR is complete; it builds successfully here, and I've also installed it on my test Jenkins system, upgraded SSH Slaves to 1.30.0 there, and was able to spawn and build on all the different ssh-connected vSphere templates I have set up. It's my first time contributing to this plugin, so if I've missed any typical steps, please let me know.

@medianick medianick marked this pull request as ready for review June 27, 2019 15:20
@pjdarton
Copy link
Member

1st-time or not, it looks like you've done everything right.

  • Kept code churn to a minimum too, which makes my life easier ;-)
  • Tested it in your own environment to make sure it works :-)
  • Flagged it as "draft" until you were ready.
    The only suggestion I have about any future contributions is that the PR description at the top is very terse - I had to see what the code changes were before I really understood what this was about. That wasn't insurmountable this time (as the code changes are minimal) but if you ever tackle anything requiring more than a 3-line change then a full description of the problem the PR is solving would be appreciated.

Out of interest, what was the symptom(s) of upgrading the SSH plugin to 1.30.0 without this fix?
I guess there will be a nasty exception and a failure to provision a VM ... but it'd be nice to record what that exception is so that anyone searching for that exception will then find this PR and know what they need to do...

Copy link
Member

@pjdarton pjdarton left a comment

Choose a reason for hiding this comment

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

Changes look good to me.

@medianick
Copy link
Contributor Author

@pjdarton -- thank you. I'd added the link to the corresponding Jira ticket in the body of my first commit message -- meaning for that to become part of the PR description -- but then did a catch-up with origin first, which resulted in needing to fill in the PR description manually, so I lost that. I've added it back. You can see the stack trace, etc. on that ticket.

@medianick medianick changed the title Pass SshHostKeyVerificationStrategy to SSHLauncher [JENKINS-57967] Pass SshHostKeyVerificationStrategy to SSHLauncher Jun 27, 2019
@pjdarton pjdarton merged commit b7e2348 into jenkinsci:master Jul 8, 2019
@pjdarton
Copy link
Member

pjdarton commented Jul 8, 2019

Plugin version 2.20 has been built & uploaded; it should be on the update servers soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants