#11626 fix twist conch --auth=sshkey

[glyph]

Scope and purpose

Fixes #11626

Contributor Checklist:

This process applies to all pull requests - no matter how small.
Have a look at our developer documentation before submitting your Pull Request.

Below is a non-exhaustive list (as a reminder):

• The title of the PR should describe the changes and starts with the associated issue number, like “#9782 Remove twisted.news. #1234 Brief description”.
• A release notes news fragment file was create in src/twisted/newsfragments/ (see: Release notes fragments docs.)
• The automated tests were updated.
• Once all checks are green, request a review by leaving a comment that contains exactly the string please review.
  Our bot will trigger the review process, by applying the pending review label
  and requesting a review from the Twisted dev team.

[glyph]

please review

[glyph]

It would be nice to address the coverage gap here, but the types are sufficient in this case (I've tested this interactively, and ssh clients can indeed authenticate to conch again)

[adiroiban]

@glyph is it ok to merge a change without a test?
In the past we were rejecting PR is they don't have tests for the changes done in that particular PR.

Regards

[glyph]

@adiroiban I suppose it's up to you, as the reviewer, if you want to say "no, we must have test coverage" ;-).

As a matter of policy we require tests. As a matter of principle I'd say "I just added type annotations, no behavior was changed" is an acceptable reason to that requirement. But of course, this change doesn't adhere to that principle; I do encoding where there wasn't any encoding before, to address the bug.

My reasoning for this fairly unprincipled exception is:

• a real test of this behavior would be exceedingly expensive, and require like… an entire docker container with a twist running as root since pwd is otherwise inaccessible, not to mention the ability to create an arbitrary home directory, so the best we could do would be a fairly unconvincing fake, which strikes me as not worth it
• this code was previously changed (and regressed) somehow, since pubkey auth used to work on python3 in this configuration; I know because I run a server whose SSH access broke
• the code just obviously doesn't work at all right now and this is a quick fix

but, you could make a similar case for any other too-long-regressed bug, so 🤷 I'll have to leave it up to a reviewer

[glyph]

• a real test of this behavior would be exceedingly expensive, and require like… an entire docker container with a twist running as root since pwd is otherwise inaccessible, not to mention the ability to create an arbitrary home directory, so the best we could do would be a fairly unconvincing fake, which strikes me as not worth it

well I guess the fake version is okay if everything is typechecked everywhere, so I added a bunch more types

[glyph]

OK, tests cover it. @adiroiban I think this might be more of a rubber stamp now :)

[adiroiban]

Many thanks for this great update.
Should be ready to merge.

[glyph]

Thanks for the prompting on the test!