Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

logging: passthrough accepts a tty #465

Merged
merged 1 commit into from
Dec 1, 2023
Merged

logging: passthrough accepts a tty #465

merged 1 commit into from
Dec 1, 2023

Conversation

giuseppe
Copy link
Member

it works in a similar way to passthrough, the only difference is that it is also allowed on a TTY.

@giuseppe giuseppe mentioned this pull request Nov 24, 2023
Merged
@rhatdan
Copy link
Member

rhatdan commented Nov 25, 2023

LGTM

@haircommander
Copy link
Collaborator

what if we had conmon set this up if -t is set with -l passthrough? why the separate option?

@giuseppe
Copy link
Member Author

what if we had conmon set this up if -t is set with -l passthrough? why the separate option?

conmon should not set a new tty. The new option is not really needed, we could reuse passthrough and drop the check we've in place there, but I preferred to make it explicit because silently passing down a tty can be dangerous. With -l passthrough-tty it is clear that the user wants that and they are ready for the risks.

@giuseppe
Copy link
Member Author

are you fine with the separate option or should I reuse "passthrough"?

@haircommander
Copy link
Collaborator

I still lean towards passthrough, as the behavior on conmon side is the same. I think it could be up to podman to warn the user about the risks. WDYT?

@giuseppe
Copy link
Member Author

giuseppe commented Dec 1, 2023

I've added the check both to Podman and conmon to play safe, so even if we mess up in Podman we still have a safety net, but I am fine if you want to keep it simpler and have only one option.

@giuseppe
logging: -l passthrough accepts TTYs
03b64fa 
enable usage also on TTYs, the caller is responsible to use it safely.

Signed-off-by: Giuseppe Scrivano <[email protected]>
@giuseppe
Copy link
Member Author

giuseppe commented Dec 1, 2023

changed to use only passthrough

@giuseppe giuseppe changed the title logging: new mode -l passthrough-tty logging: passthrough accepts a tty Dec 1, 2023
@giuseppe giuseppe marked this pull request as ready for review December 1, 2023 18:57
@rhatdan rhatdan merged commit 951229b into containers:main Dec 1, 2023
16 of 18 checks passed
giuseppe added a commit to giuseppe/libpod that referenced this pull request Feb 28, 2024
@giuseppe
logging: new mode -l passthrough-tty
950f612 
it works in a similar way to passthrough but it allows to be used also
on a TTY.

conmon support: containers/conmon#465

Closes: containers#20767

Signed-off-by: Giuseppe Scrivano <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@giuseppe @rhatdan @haircommander