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.

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

Merged
merged 2 commits into from
Feb 9, 2024
Merged

kubetui 1.5.0 (new formula) #161155

merged 2 commits into from
Feb 9, 2024

Conversation

sarub0b0
Copy link
Contributor

@sarub0b0 sarub0b0 commented Jan 28, 2024

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@github-actions github-actions bot added new formula PR adds a new formula to Homebrew/homebrew-core rust Rust use is a significant feature of the PR or issue labels Jan 28, 2024
Copy link
Contributor

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.

@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label Jan 28, 2024
@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label Jan 28, 2024
Comment on lines 17 to 18
assert_match "An intuitive Terminal User Interface (TUI) tool",
shell_output("#{bin}/kubetui --help")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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.

Formula/k/kubetui.rb Outdated Show resolved Hide resolved
@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label Jan 28, 2024
@sarub0b0 sarub0b0 force-pushed the sarub0b0 branch 2 times, most recently from 915fadb to 2d2c2f3 Compare January 29, 2024 13:06
@chenrui333
Copy link
Member

@sarub0b0 please squash all the commits?

@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label Feb 1, 2024
@sarub0b0 sarub0b0 force-pushed the sarub0b0 branch 2 times, most recently from d3308eb to b7cb75f Compare February 1, 2024 02:44
@sarub0b0
Copy link
Contributor Author

sarub0b0 commented Feb 1, 2024

@chenrui333
I squashed commits. Please review. Thank you.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ohai cmd

Formula/k/kubetui.rb Outdated Show resolved Hide resolved
# 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")
Copy link
Member

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?

Copy link
Contributor Author

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.

actions/runner#241

Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Contributor Author

@sarub0b0 sarub0b0 Feb 6, 2024

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.

@sarub0b0 sarub0b0 force-pushed the sarub0b0 branch 3 times, most recently from f65ba39 to 704c492 Compare February 6, 2024 15:44
Formula/k/kubetui.rb Outdated Show resolved Hide resolved
@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label Feb 6, 2024
@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label Feb 6, 2024
@sarub0b0
Copy link
Contributor Author

sarub0b0 commented Feb 7, 2024

I have made changes to the code, so please review it.

Copy link
Contributor

github-actions bot commented Feb 9, 2024

@chenrui333
Copy link
Member

@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!!!!

@github-actions github-actions bot added the CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. label Feb 9, 2024
@BrewTestBot BrewTestBot added this pull request to the merge queue Feb 9, 2024
Merged via the queue into Homebrew:master with commit 8832572 Feb 9, 2024
12 checks passed
@github-actions github-actions bot added the outdated PR was locked due to age label Mar 10, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age rust Rust use is a significant feature of the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants