Skip to content

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

Merged
maxdymond merged 1 commit intoMetaswitch:mainfrom
olipratt:olipratt/fix-no-tty
Apr 18, 2023
Merged

Don't add -i to docker command when stdin is not a TTY#280
maxdymond merged 1 commit intoMetaswitch:mainfrom
olipratt:olipratt/fix-no-tty

Conversation

@olipratt
Copy link
Copy Markdown
Contributor

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

  • New/modified Rust code formatted with cargo fmt
  • Documentation and README.md updated for this change, if necessary
  • CHANGELOG.md updated for this change

@maxdymond
Copy link
Copy Markdown
Member

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 olipratt force-pushed the olipratt/fix-no-tty branch from e5cc6fc to 8c0ad03 Compare April 14, 2023 10:09
@olipratt
Copy link
Copy Markdown
Contributor Author

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

Thanks!

Signed-off-by: olipratt <olipratt@users.noreply.github.com>
@olipratt olipratt force-pushed the olipratt/fix-no-tty branch from 8c0ad03 to 9d66d29 Compare April 18, 2023 13:26
@maxdymond maxdymond merged commit 60ebac4 into Metaswitch:main Apr 18, 2023
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.

2 participants