tkconch: 'IPv4Address' object does not support indexing

[teratorn]

Error in Twisted trunk r36406 tkconch script when clicking 'Connect' in GUI. Attempted to connect to a local conch manhole server.

  File "/home/teratorn/data/code/TX/lib/python2.6/site-packages/twisted/internet/posixbase.py", line 614, in _doReadOrWrite
    why = selectable.doRead()
  File "/home/teratorn/data/code/TX/lib/python2.6/site-packages/twisted/internet/tcp.py", line 215, in doRead
    return self._dataReceived(data)
  File "/home/teratorn/data/code/TX/lib/python2.6/site-packages/twisted/internet/tcp.py", line 221, in _dataReceived
    rval = self.protocol.dataReceived(data)
  File "/home/teratorn/data/code/TX/lib/python2.6/site-packages/twisted/conch/ssh/transport.py", line 438, in dataReceived
    self.dispatchMessage(messageNum, packet[1:])
  File "/home/teratorn/data/code/TX/lib/python2.6/site-packages/twisted/conch/ssh/transport.py", line 453, in dispatchMessage
    f(payload)
  File "/home/teratorn/data/code/TX/lib/python2.6/site-packages/twisted/conch/ssh/transport.py", line 1151, in ssh_KEX_DH_GEX_GROUP
    return self._ssh_KEXDH_REPLY(packet)
  File "/home/teratorn/data/code/TX/lib/python2.6/site-packages/twisted/conch/ssh/transport.py", line 1130, in _ssh_KEXDH_REPLY
    d = self.verifyHostKey(pubKey, fingerprint)
  File "/home/teratorn/data/code/TX/lib/python2.6/site-packages/twisted/conch/scripts/tkconch.py", line 374, in verifyHostKey
    if options['host'] == self.transport.getPeer()[1]:
exceptions.TypeError: 'IPv4Address' object does not support indexing

[Julian]

Hey :). Thanks for submitting this.

I think it looks like getitem was deprecated and removed on IPv4Address in #4817. I don't think the correct fix is to add it back, the script probably needs updating to use attributes like the deprecation recommends.

[exarkun]

Also, note that all changes need to be unit tested. Thanks!

[borko]

@Julian: thx, the task didn't precise what to do. So as I understand we want to replace all indexing operators by the dot operator ? @exarkun: I don't see any tests for conch, so you want me to create new suite?

[exarkun]

Conch has tests in twisted/conch/test. However, if you meant you couldn't find any tests for tkconch, then I think you're on the right track: there don't appear to be very many (there is one, or perhaps two, in twisted/conch/test/test_scripts.py, but I don't think those will be very helpful to you, since they seem to be focused on whether the script is runnable, rather than any particular SSH-related behavior it might have). So, yes, a new test module is probably in order.

As far as the other part goes, yes - address[0] should be replaced with address.host and address[1] should be replaced with address.port.

[borko]

1. Deleted places that were using indexing operator. 2. So this task is about reverting changes therefore I think writing tests for this is pointless I guess.

[teratorn]

Hi borko,

Thank you for working on this. Sorry that tkconch is so fragile - but the main reason that it *is* so fragile, and is clearly undermaintained, and has been broken, in similar ways, several times before, is BECAUSE there is no test coverage for any of this code.

That has to be the primary focus of this ticket, honestly. Otherwise we should just get rid of tkconch entirely.

I understand if you just wanted to submit a simple patch that "fixes the bugs", but there is very little point if we don't do SOMETHING to improve the maintainability of tkconch moving forward.

I HOPE that we can get this issue fixed, without having to force someone to write 100% line/branch coverage for every module touched (as especially considering this is *Tk* *GUI* code) as that seems completely daunting compared to the motivation that a contributor is going to bring to bear against an issue of this size.

But I have no authority to unilaterally bend the rules for you - our development process dictates that 100% test coverage be implemented, even if the modified code had no tests to begin with.

This is how we are able to move Twisted forward toward having better test coverage for old modules (like this) that have been around for ages, and are in a state of disrepair. Perhaps you can solicit further opinions on #twisted or the mailing list.

[teratorn]

Alright, I guess I've been a little confused on what our actual testing requirement is for changed code. You are only required to add test for changed lines, not necessarily entire modules.