[JENKINS-73427] With 'Accept first connection' host key verification, and JGit, newly added known_hosts entries are malformed

[akash-manna-sky]

Fixes #1686

JENKINS-73427

With 'Accept first connection' host key verification, and JGit, newly added known_hosts entries are malformed

Testing done

Submitter checklist

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests that demonstrate the feature works or the issue is fixed

[akash-manna-sky]

Hello @MarkEWaite, can you please review this PR?

[MarkEWaite]

Hello @MarkEWaite, can you please review this PR?

Needs one or more tests that show the issue and confirm that the issue is resolved by the code change. In this case, it may be better to not pass the HashKnownHosts argument at all so that users can configure it in their ssh_config if they really want it. Refer to issue report:

• [JENKINS-76261] Please reconsider forcing HashKnownHosts=yes #1702

[akash-manna-sky]

Can you please review the changes? @MarkEWaite

[MarkEWaite]

First review comments. More in another day

[MarkEWaite]

If it is expected to throw an exception, please use assertThrows

[akash-manna-sky]

Please have a look. @MarkEWaite

[MarkEWaite]

@akash-manna-sky I would like to test this in my development environment, but I'm not allowed to merge the master branch to it. Could you merge the master branch into it so that the latest fixes and releases are included?

[akash-manna-sky]

@akash-manna-sky I would like to test this in my development environment, but I'm not allowed to merge the master branch to it. Could you merge the master branch into it so that the latest fixes and releases are included?

I rebase my branch with master branch, so all the latest fixes and releases are included now. You can test it now.

[MarkEWaite]

I rebase my branch with master branch, so all the latest fixes and releases are included now. You can test it now.

Thanks. Generally it is better to merge rather than rebase in a pull request. When you rebase, it often disconnects comments from the changes. The change will be squash merged eventually, so the various steps in the pull request commit history will be collapsed into a single commit on the master branch.

[akash-manna-sky]

Have you tested the changes? Please tell me if anything I need to modify or update. @MarkEWaite

[MarkEWaite]

Have you tested the changes? Please tell me if anything I need to modify or update. @MarkEWaite

No issues detected in my testing thus far, but I'm testing mostly by using the plugin in other work. I will need to do more specific testing by changing the host key verification strategy to look for surprises.

[akash-manna-sky]

Have you tested the changes? Please tell me if anything I need to modify or update. @MarkEWaite

No issues detected in my testing thus far, but I'm testing mostly by using the plugin in other work. I will need to do more specific testing by changing the host key verification strategy to look for surprises.

Thanks for testing! That makes sense. I’ll wait for your additional host key verification testing—please let me know if you’d like me to make changes on my side. @MarkEWaite

[MarkEWaite]

Two minor changes to test assertions based on my review from inside a debugger.

[akash-manna-sky]

Is this PR fine now ? Please let me know if there’s anything I need to update. @MarkEWaite

[MarkEWaite]

Is this PR fine now ? Please let me know if there’s anything I need to update. @MarkEWaite

I'm still testing and exploring.

[MarkEWaite]

It has passed all my tests. Code review prompted me to make a slight improvement in coverage with another test, but otherwise I think it is ready to be tested again in plugin BOM and tested in the acceptance test harness.

[akash-manna-sky]

It is good to see that my changes have passed all the tests and have been merged. Please assign or suggest me a few issues in this plugin, or in other plugins you maintain, that are higher priority to fix and for which you are willing to accept new pull requests, so that I can continue contributing. I am especially interested in back-end–related tasks. Thank you!

@MarkEWaite

[MarkEWaite]

Please assign or suggest me a few issues in this plugin, or in other plugins you maintain,

No, I won't. I'm growing weary of your repeated requests.

Please close pull requests that you're not continuing, like:

• Windows agents connected to Jenkins by SSH hang when cloning an SSH repo from github.com #1716

Please invest more of your time to deeply test the changes you're proposing. Describe the ways that you've deeply tested those proposed changes. Show that you've exhausted all your own ideas for testing the changes that you are proposing. Then allow time for the maintainers to review them.

In the case of this specific pull request, there was no clear evidence in the pull request description that you were able to duplicate the issue yourself. You disabled hashing of the known hosts file, but didn't describe how you had tested it yourself. I spent multiple hours performing various tests trying to assure that the change is ready to ship.

Please allow changes from maintainers on your pull request branches. When you disallow changes from maintainers, you make the interaction more complicated for maintainers that are reviewing your proposed changes.

Please reduce the frequency at which you ask for updates. Most maintainers will ignore you if you request too frequently or they will ask that you be banned from the organization.