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

properly check for tty when in powershell #1607

Merged
merged 1 commit into from
Sep 1, 2020
Merged

properly check for tty when in powershell #1607

merged 1 commit into from
Sep 1, 2020

Conversation

vilmibm
Copy link
Contributor

@vilmibm vilmibm commented Aug 31, 2020

closes #1581

Only under Powershell we were failing to accurately tell if a command was running attached to a tty or not due to our use of colorableOut. This PR tweaks the check to account for this backports a change made in #1552 that cached the isTerminal check prior to creating a colorable, avoiding the bug.

I had to do a gross hack to fix this and am open to suggestion. colorable only builds the colorable.Writer type if built on windows; this made detecting the above scenario very difficult. I had to make a shim type that is conditionally built in order to get this compiling on all of our CI test nodes.

@vilmibm vilmibm added this to Backlog 🗒 in The GitHub CLI via automation Aug 31, 2020
@vilmibm vilmibm requested a review from mislav August 31, 2020 21:31
@vilmibm vilmibm moved this from Backlog 🗒 to Needs review 🤔 in The GitHub CLI Aug 31, 2020
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Nice catch! We can't use IsTerminal on a colorable stream; that will always return false even if it would return true for the original stream.

It looks like the release-cmd branch might inadvertently fix this due to the changes to iostreams.System() I've made there: https://github.com/cli/cli/pull/1552/files#diff-9e30abd45545c3e865502cae5bf624d9

There, we first check IsTerminal and only after that we convert the streams to colorable. That approach does not require us to add any build tags. What do you think?

@vilmibm
Copy link
Contributor Author

vilmibm commented Sep 1, 2020

I'm totally fine with that solution! I do not like the build tags hack at all.

I for a second thought that the isTerminal check wouldn't work on powershell anyway, but I just tested it and that will work fine.

However, we wanted to release today, right? It seems unwise to rush releases in today and I'd love to not have things so broken on powershell; can you backport the iostreams changes?

@vilmibm
Copy link
Contributor Author

vilmibm commented Sep 1, 2020

I'll leave this open as a reminder but we're just going to focus on getting #1552 in as the fix for this issue.

nevermind, I'll backport the fix from 1552

The GitHub CLI automation moved this from Needs review 🤔 to Needs to be merged 🎉 Sep 1, 2020
@vilmibm vilmibm merged commit bf9d3ab into trunk Sep 1, 2020
The GitHub CLI automation moved this from Needs to be merged 🎉 to Pending Release 🥚 Sep 1, 2020
@mislav mislav deleted the pwsh-tty branch September 2, 2020 11:14
@github-actions github-actions bot moved this from Pending Release 🥚 to Done 💤 in The GitHub CLI Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
The GitHub CLI
  
Done 💤
Development

Successfully merging this pull request may close these issues.

gh repo fork --remote works differently in powershell v command line
2 participants