Skip to content

Conversation

asmodehn
Copy link
Contributor

@asmodehn asmodehn commented Aug 28, 2025

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • I have updated the documentation.

Reference to Existing Issue

Addresses openwisp/openwisp-firmware-upgrader#244 (comment)

Description of Changes

Paramiko recv_exit_status() doesnt accept timeout as parameter, and might wait until deconnection, even if that is not necessary.

This is a three line workaround that problem that only wait the SSH_COMMAND_TIMEOUT

@asmodehn asmodehn marked this pull request as draft August 28, 2025 09:35
@asmodehn asmodehn force-pushed the fix_ignored_command_timeout branch from a265fa0 to f2c76e7 Compare August 29, 2025 09:51
@asmodehn
Copy link
Contributor Author

I guess an improvement could be to wait only the time left in SSH_COMMAND_TIMEOUT after the command is run, but that might be add a bit of logic to exec_command...

@coveralls
Copy link

coveralls commented Aug 29, 2025

Coverage Status

coverage: 98.744% (-0.02%) from 98.759%
when pulling 24ec502 on asmodehn:fix_ignored_command_timeout
into 3a45fd9 on openwisp:master.

@asmodehn asmodehn marked this pull request as ready for review August 29, 2025 12:07
@asmodehn
Copy link
Contributor Author

FYI : It looks like, after this change, we could get rid of the subprocess there: https://github.com/openwisp/openwisp-firmware-upgrader/blob/master/openwisp_firmware_upgrader/upgraders/openwrt.py#L414
I understand it becomes redundant ?

@nemesifier
Copy link
Member

FYI : It looks like, after this change, we could get rid of the subprocess there: https://github.com/openwisp/openwisp-firmware-upgrader/blob/master/openwisp_firmware_upgrader/upgraders/openwrt.py#L414 I understand it becomes redundant ?

I had to implement it using subprocess because in old openwrt versions sysupgrade just hanged, it was so annoying.

Maybe with this solution, we won't need to do that anymore, we'll have to test.

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.

3 participants