Skip to content

fix(spin): preserve color output when --show-output is given#850

Merged
andreynering merged 1 commit intomainfrom
spin-preserve-color-output-show-output-flag
Feb 11, 2025
Merged

fix(spin): preserve color output when --show-output is given#850
andreynering merged 1 commit intomainfrom
spin-preserve-color-output-show-output-flag

Conversation

@andreynering
Copy link
Copy Markdown
Member

@andreynering andreynering commented Feb 10, 2025

Fixes #305

Running the command inside a PTY (using this package) to ensure colors and general formatting are preserved.

@andreynering andreynering self-assigned this Feb 10, 2025
@andreynering andreynering requested a review from a team as a code owner February 10, 2025 17:01
@andreynering andreynering requested review from raphamorim and removed request for a team February 10, 2025 17:01
@andreynering andreynering force-pushed the spin-preserve-color-output-show-output-flag branch from f7ffddc to 9eaccca Compare February 10, 2025 17:25
Comment thread spin/pty.go Outdated
@andreynering andreynering force-pushed the spin-preserve-color-output-show-output-flag branch 2 times, most recently from 7588eef to b93a075 Compare February 11, 2025 19:26
Copy link
Copy Markdown
Contributor

@raphamorim raphamorim left a comment

Choose a reason for hiding this comment

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

code lgtm, I was just gonna ask about windows but pinged @andreynering instead

@andreynering andreynering force-pushed the spin-preserve-color-output-show-output-flag branch from b93a075 to da77e9a Compare February 11, 2025 19:33
Copy link
Copy Markdown
Contributor

@aymanbagabas aymanbagabas left a comment

Choose a reason for hiding this comment

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

Few nitpicks, but LGTM otherwise!

Comment thread spin/spin.go
if err = stdoutPty.Start(executing); err != nil {
return errorMsg(err)
}
_ = xpty.WaitProcess(context.Background(), executing)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just an FYI, we don't need to use xpty.WaitProcess here, we can call executing.Wait() directly. That's what it does under the hood.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@aymanbagabas I decided to keep as is so the code is more prepared to work on Windows if we decide to try again in the future. WDYT?

Comment thread spin/spin.go
Comment thread spin/spin.go
Comment on lines +79 to +85
if isTerminal && runtime.GOOS == "windows" {
executing.Stdout = io.MultiWriter(&bothbuf, &outbuf)
executing.Stderr = io.MultiWriter(&bothbuf, &errbuf)
_ = executing.Run()
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

After some headache, me and @aymanbagabas decided to keep the old behavior on Windows for now.

(Git Bash was freezing for some reason, although the other terminals worked correctly).

We can reconsider in the future if we find a reliable way to make it work on Windows.

@andreynering andreynering force-pushed the spin-preserve-color-output-show-output-flag branch from da77e9a to f66c0ba Compare February 11, 2025 19:47
@andreynering andreynering merged commit 93f6857 into main Feb 11, 2025
@andreynering andreynering deleted the spin-preserve-color-output-show-output-flag branch February 11, 2025 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Preserve output formatting with spin --show-output

3 participants