Code review comment for ~ajorgens/cloud-init:pipe-cat

Revision history for this message
Scott Moser (smoser) wrote :

Andrew,
thanks. I think the code looks good
But a few things

a.) maybe 'detach_tty' as the name? 'no_tty=False' seems to imply that there would be a tty, when we are not actually creating one.
b.) need some sort of test, at least to push it through here. it might be tricky to do, but i'd like to run a subp 'detach_tty=False' and get some subprocess to believe it is a tty.
   subp(['bash', '-c', '[ "-t" 1 ] && echo attached to tty || echo not attached to tty'])
   the difficulty is that in any c-i or automated environment, we're probably not attached to a tty. but we do need some test of this code path.
c.) i think best to take the detach_tty path only if we *have* a tty.
   if os.isatty(sys.stdout):
      ... go the detach route
   else:
      ... no need to detach

« Back to merge proposal