Metaswitch / floki

Launch containers to help build your software
MIT License
66 stars 21 forks source link

Don't add -i to docker command when stdin is not a TTY #280

Closed olipratt closed 1 year ago

olipratt commented 1 year ago

Why this change?

Hi!

This fixes uses of floki in script environments where stdout appears to be a TTY, but stdin is not.

Relevant testing

The most basic repro scenario of the problem is:

$ echo "" | floki
the input device is not a TTY
10:21:41 [ERROR] A problem occurred: Running container failed: docker run exited with return code 1

(I'm not actually trying to pipe anything into floki - the tool I'm using which wants to run floki just doesn't have stdin as a TTY).

Running this with the fix means the above command (and my specific scripted scenario) works correctly.

Didn't see a way to add any regression test to cover this case.

Should be perfectly safe - in any case where stdin was not a TTY, which is the only case this impacts, then floki would have failed.

Contributor notes

If you accept, could you please release a new version with this fix included, so that I can get this working for my use case?

Thanks!

Checks

maxdymond commented 1 year ago

Looking at https://www.baeldung.com/linux/docker-run-interactive-tty-options it seems that because we specify -t by default, we absolutely should be checking whether the stdin is a tty. So I'm pretty sure this fix does the correct thing.

olipratt commented 1 year ago

@maxdymond - I fixed up the misssing 'signed-off-by' in the commit - are you happy to accept and create a new release?

Thanks!