-
Notifications
You must be signed in to change notification settings - Fork 246
Do not run with --tty when not in interactive mode #1506
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.
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
Conversation
Reviewer's GuideThis PR updates the TTY allocation logic in the engine to skip the -t flag when not in interactive mode or when user arguments are provided, and extends the system tests to verify correct inclusion or exclusion of the -t flag under different run scenarios. Sequence Diagram: Conditional TTY Flag for Podman ExecutionsequenceDiagram
actor User as CLI User
participant RE as Ramalama Engine
participant P as Podman
CLI User->>RE: Executes command
RE->>RE: Evaluates TTY conditions (add_tty_option)
alt (NOT self.args.ARGS) AND sys.stdin.isatty()
RE->>P: podman run ... -t ...
else (self.args.ARGS) OR (NOT sys.stdin.isatty())
RE->>P: podman run ... (no -t) ...
end
Class Diagram: Update to RamalamaEngineclassDiagram
class RamalamaEngine {
+add_tty_option()
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
This should help diagnose issues like: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @rhatdan - I've reviewed your changes - here's some feedback:
- In add_tty_option you dropped the stdout.isatty check—consider re-including it so you only request a TTY when both stdin and stdout are actual terminals to avoid forcing a TTY on redirected output.
- The new tests for non‐interactive runs still assert that "-t" is present—but they should be asserting that the "-t" flag is absent when ARGS are provided or stdin isn’t a TTY.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ramalama/engine.py
Outdated
def add_tty_option(self): | ||
if sys.stdout.isatty() or sys.stdin.isatty(): | ||
self.exec_args += ["-t"] | ||
if self.args.ARGS or not sys.stdin.isatty(): | ||
return | ||
self.exec_args += ["-t"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider adding -i
for interactive input
The -t
flag alone allocates a TTY, but to keep STDIN open for interactive use, you should also add -i
(i.e., use -it
).
def add_tty_option(self): | |
if sys.stdout.isatty() or sys.stdin.isatty(): | |
self.exec_args += ["-t"] | |
if self.args.ARGS or not sys.stdin.isatty(): | |
return | |
self.exec_args += ["-t"] | |
def add_tty_option(self): | |
if self.args.ARGS or not sys.stdin.isatty(): | |
return | |
self.exec_args += ["-i", "-t"] |
69578c5
to
5315b2d
Compare
CI failing |
I have found that when running with nvidia the -t (--tty) option in podman is covering up certain errors. When we are not running ramalama interactively, we do not need this flag set, and this would make it easier to diagnose what is going on with users systems. Don't add -i unless necessary Server should not need to be run with --interactive or --tty. Signed-off-by: Daniel J Walsh <[email protected]>
I have found that when running with nvidia the -t (--tty) option in podman is covering up certain errors. When we are not running ramalama interactively, we do not need this flag set, and this would make it easier to diagnose what is going on with users systems.
Summary by Sourcery
Disable the automatic inclusion of the TTY flag for non-interactive executions to prevent masking errors and update system tests to validate this behavior.
Bug Fixes:
Enhancements:
Tests: