-
-
Notifications
You must be signed in to change notification settings - Fork 12.3k
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
kubetui 1.5.0 (new formula) #161155
kubetui 1.5.0 (new formula) #161155
Conversation
Thanks for contributing to Homebrew! 🎉 It looks like you're having trouble with a CI failure. See our contribution guide for help. You may be most interested in the section on dealing with CI failures. You can find the CI logs in the Checks tab of your pull request. |
Formula/k/kubetui.rb
Outdated
assert_match "An intuitive Terminal User Interface (TUI) tool", | ||
shell_output("#{bin}/kubetui --help") |
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.
assert_match "An intuitive Terminal User Interface (TUI) tool", | |
shell_output("#{bin}/kubetui --help") | |
output = shell_output("#{bin}/kubetui --context not_exist 2>&1", 1) | |
assert_match "failed to read kubeconfig", output |
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.
I am encountering the following error in Linux CI, so I am considering skipping the startup check tests similar to other formulas. Is it acceptable to do so?
Panic! disable raw mode
thread 'main' panicked at src/main.rs:180:5:
failed to enable raw mode: Os { code: 6, kind: Uncategorized, message: "No such device or address" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
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.
That looks like a bug in the upstream code, it should be reported there and fixed.
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.
I modified the test to use the script
command because kubetui was unable to set the tty to raw mode in the Linux CI job, resulting in an error.
915fadb
to
2d2c2f3
Compare
@sarub0b0 please squash all the commits? |
d3308eb
to
b7cb75f
Compare
@chenrui333 |
Formula/k/kubetui.rb
Outdated
if OS.linux? && ENV["HOMEBREW_GITHUB_ACTIONS"] | ||
# Use script command because it fails with: | ||
# failed to enable raw mode: Os { code: 6, kind: Uncategorized, message: "No such device or address" } | ||
ohai cmd |
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.
ohai cmd |
Formula/k/kubetui.rb
Outdated
# failed to enable raw mode: Os { code: 6, kind: Uncategorized, message: "No such device or address" } | ||
ohai cmd | ||
|
||
r, w, pid = PTY.spawn("script -q -e -c \"#{cmd}\" screen.log") |
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.
Why not just run the command directly?
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.
Thank you for the review.
kubetui needs to change the terminal to raw mode, but Linux CI does not have a tty. Therefore, I have worked around this issue by using the script command.
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.
But you're also running it in a pseudo tty. Doesn't that work?
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.
Is it with PTY.spawn
?
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.
Yes, pty is pseudo tty
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.
I tried using PTY without the script
command, and it worked successfully. I have pushed the modified code, so please review it.
f65ba39
to
704c492
Compare
I have made changes to the code, so please review it. |
@sarub0b0, thanks for your first contribution to homebrew-core! 🎉 🥇 Without awesome contributors like you, it would be impossible to maintain Homebrew to the high level of quality users have come to expect. Thank you!!!! |
HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>
, where<formula>
is the name of the formula you're submitting?brew test <formula>
, where<formula>
is the name of the formula you're submitting?brew audit --strict <formula>
(after doingHOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>
)? If this is a new formula, does it passbrew audit --new <formula>
?