fix wrong compression applied ('none',) when compress = True, but no compression tuple provided to cnopts - #81
Conversation
There was a problem hiding this comment.
Allowing the default compression algorithm list set by paramiko is fine and I see your point from a user's perspective of thinking something is enabled only to have it not be. I'm not sure about enabling the compression test in CI though. Also unsure about the benefit of th security_options line as everything is within the pep8 line width already and direct calling attributes of the object doesn't reinit it every time as far as I'm aware, though aesthetically I do think it may looks better.
…fter the other for better readability
byteskeptical
left a comment
There was a problem hiding this comment.
Compression changes/ line moves look good. Lets see what happens with the test on actions. Will need you to add those secrets to your fork for all OS platforms to run correctly (Linux & Windows).
byteskeptical
left a comment
There was a problem hiding this comment.
Thanks again, everything looks good, I did end up moving the enable/disable compress lines back out as I realized that I have originally done that to seperate the settings set on the transport itself from the ones set on the security options object.

In the current version of the code,
test_compression_enabledfails and it's been disabled in CI tests. I was able to find the issue and implement a fix (also enabled in CI tests).In the
_start_transportmethod, ifcnopts.compressis True, it callsself._transport.use_compression(compress=bool(compress)). This sets the compression options within paramiko Transport -After this step however, if
cnopts.compressionis not provided, it defaults to ("none",), and_start_transportmethod assigns this back into the transport through security options. This resets paramiko's_preferred_compressionto ("none",) and disables compression. This is effectively equivalent toself._transport.use_compression(compress=False).It also makes the behavior inaccurate per cnopts docstring which states that the default
cnopts.compressionisparamiko.Transport.SecurityOptions.compressionThe fix is to only set security options compression if cnopts compress is enabled and compression is not the default value ('none',)
Additionally, I have created one instance of security options so the object is not initialized multiple times for each option. This also keeps line width within pep8 recommendations