search

canonical / pebble

Take control of your internal daemons!
GNU General Public License v3.0
137 stars 52 forks source link

fix(enter): disable logs for interactive exec runs #347

Closed rebornplusplus closed 4 months ago

rebornplusplus commented 5 months ago

Fixes #339

The PR disables logs for pebble enter exec only if the exec command is connected to a TTY and it makes the TTY RAW, even if the --verbose flag is passed.

Interactive applications, like Vim, can be run as an exec command:

pebble enter --verbose exec -it vim

Since enter runs the pebble daemon and executes the exec command afterwards, the same TTY that will be made raw by exec to run interactive applications like Vim is also used for logging and service outputs. Thus, the logs coming from pebble find the TTY RAW and weird indentation is introduced. See issue #339.

The issue, however, is not limited to only indentation. For example, if a text editor was being run with exec -it, like Vim, the incoming logs would go to the text editor and may change the content of the editor’s buffer.

This PR disables logs completely for pebble enter --verbose exec -it (if a terminal is connected) even if the --verbose flag is passed. Since logs are currently disabled for pebble enter exec if --verbose is not passed and -i, -t are assumed if stdin and stdout are TTYs, the PR only ignores the --verbose flag in cases where exec is connected to a TTY and it will make the TTY raw.

See also the spec, RK034.

rebornplusplus commented 5 months ago

Given that there's no tests for this (and I think tests would be hard to write), can you confirm you've tested pebble exec and pebble enter for the various cases?

Yup, I can confirm that I have tested pebble enter and pebble exec. There are no behavioural changes for pebble exec alone. Only pebble enter [..] exec is affected.

rebornplusplus commented 5 months ago

I've provided some questions in the spec, about the use of FD 2, and about some inconsistencies in the proposal related to the use of the flags vs. the use of the actual terminal state.

I have updated the proposal to be more clear in the spec and the PR. Thanks!

rebornplusplus commented 4 months ago

Closing in favor of below:

We will be removing --verbose from Rocks entrypoint. So, Rocks will now have only pebble enter as entrypoint. Additionally, we will also be BLOCKING the use of --verbose with pebble enter exec. So, pebble enter --verbose [...] exec [...] <cmd...> will produce an error.