Opened 3 years ago

Closed 2 years ago

#8232 enhancement closed duplicate (duplicate)

Port twisted.conch.ssh.transport to Python 3

Reported by: hawkowl Owned by: hawkowl
Priority: normal Milestone: Python-3.x
Component: conch Keywords:
Cc: z3p Branch: conch-transport-py3-8232-4
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl


Change History (9)

comment:1 Changed 3 years ago by DefaultCC Plugin

Cc: z3p added

comment:2 Changed 3 years ago by hawkowl

Author: hawkowl
Branch: branches/conch-transport-py3-8232

(In [47002]) Branching to conch-transport-py3-8232.

comment:3 Changed 3 years ago by hawkowl

Branch: branches/conch-transport-py3-8232branches/conch-transport-py3-8232-2

(In [47045]) Branching to conch-transport-py3-8232-2.

comment:4 Changed 3 years ago by hawkowl

Branch: branches/conch-transport-py3-8232-2conch-transport-py3-8232-2

comment:5 Changed 3 years ago by hawkowl

Branch: conch-transport-py3-8232-2conch-transport-py3-8232-3

comment:6 Changed 2 years ago by hawkowl

Branch: conch-transport-py3-8232-3conch-transport-py3-8232-4

comment:7 Changed 2 years ago by hawkowl

Keywords: review added

Tests pass, please review.

comment:8 Changed 2 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Many thanks for the code.

Major comments:

  1. Instead of removing test_KEXINIT_badKexAlg I think that it should be updated to describe/document the new behaviour.
  1. In twisted/conch/ssh/ is it ok to change the behaviour of the kexAlg setter? Why? I think that the docstring should inform that is set to know for unknown/unsupported algorithms and this looks like public API so we also need a release notes fragment.
  1. I see there is missing coverage for ssh_USERAUTH_PK_OK_keyboard_interactive ... now I understand why you wanted that code removed. Let me know if you need help in writing the unit tests for this code.
  1. there is no topfile fragment

Minor comments:

  1. In twisted/conch/ssh/ i think that the key for SSHFactory.sevices should be bytes to be close to what the protocol expects.... I saw that ssh/ you are callling factory.getService with nativeString but maybe we should update the getService method to do this auto-conversion
  1. I think that we should update the docstring for to inform that it should be bytes.. but ascii-only native strings are also supported and auto-converted.

please check my comments and resubmit. Thanks!

comment:9 Changed 2 years ago by hawkowl

Resolution: duplicate
Status: newclosed

Duplicate of #8638

Note: See TracTickets for help on using tickets.