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

feat: Better support for non-interactive mode #18

Merged
merged 1 commit into from
Oct 27, 2022

Conversation

jirihnidek
Copy link
Contributor

@jirihnidek jirihnidek commented Oct 10, 2022

  • When stdout is not TTY, then do not display spinner and status text and the output of rhc should not be colorful, because output is redirected to some file. When temporary status messages and escape sequences were added to log file, then viewing such file could be complicated.
  • When NO_COLOR environment variable is set, then do not display color nor animation too.
  • This commit closes Better support for non-interactive shell environments #14 issue

@jirihnidek jirihnidek force-pushed the non-interactive_shell branch from ab48252 to 3c76e58 Compare October 10, 2022 16:02
@jirihnidek
Copy link
Contributor Author

@subpop Hi, can I ask for PR review? Thanks.

@jirihnidek
Copy link
Contributor Author

jirihnidek commented Oct 14, 2022

BTW: It would be probably good to refactor the code a little bit, because some code is duplicated. There is also one huge function in main.go file, but I think it would be good to do refactoring in another PR.

Copy link
Collaborator

@subpop subpop left a comment

Choose a reason for hiding this comment

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

Looks great overall. I realize I left part of my plan for this feature out of the reporting issue. I want to include support for the NO_COLOR environment variable. See the Output section of the CLIG for more best practices around supporting non-TTY outputs.

main.go Outdated
// Detect if the output goes to TTY or some file
var isTTY bool
_, err := unix.IoctlGetWinsize(int(os.Stdout.Fd()), unix.TIOCGWINSZ)
if err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this can be simplified to

_, err := unix.IoctlGetWinsize(int(os.Stdout.Fd()), unix.TIOCGWINSZ)
isTTY := err == nil

You'll also probably want to report this error, if it is not nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it can be simplified like this. I don't think that I want to report the error (err) in this case, because it is absolutely normal, when somebody redirects stdout to some file. Or do I miss anything?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What sorts of errors do IoctlGetWinsize return? Are they fatal to the operation of the program or useful to report to the user? We do appear to be using it exclusively to check whether we're running in a TTY or not, so I can see the reason for not reporting it.

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 was thinking about this more, and maybe we should use something more reliable... What about using this?

https://pkg.go.dev/github.com/mattn/go-isatty#section-readme
GitHub repository of this project https://github.com/mattn/go-isatty

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using something more reliable is a good idea. Looking at go-isatty, it is an extremely tiny package. We could just carry the code directly, rather than using an external package.

// IsTerminal return true if the file descriptor is terminal.
func IsTerminal(fd uintptr) bool {
	_, err := unix.IoctlGetTermios(int(fd), unix.TCGETS)
	return err == nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe using go-isatty is overkill in our case, because we need support only for Linux (not for Plan9, Solaris, etc.) We can grab 3 lines of code from there and just use it.

func IsTerminal(fd uintptr) bool {
	_, err := unix.IoctlGetTermios(int(fd), unix.TCGETS)
	return err == nil
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yea. I don't mean to diminish the value that package; under certain conditions, I can agree with abstracting that kind of functionality into a package. But for our use here, calling unix.IoctlGetTermios directly seems sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Funny, I should reload page before replying to any comment and don't trust to automatic updating of GitHub webpage ... It seems we had same idea.

@jirihnidek
Copy link
Contributor Author

Looks great overall. I realize I left part of my plan for this feature out of the reporting issue. I want to include support for the NO_COLOR environment variable. See the Output section of the CLIG for more best practices around supporting non-TTY outputs.

Do you want to support only NO_COLOR environment variable or do you want to support other suggestions from Output section of the CLIG (like --no-color)?

@jirihnidek jirihnidek force-pushed the non-interactive_shell branch from 3c76e58 to a7fd750 Compare October 19, 2022 08:56
@subpop
Copy link
Collaborator

subpop commented Oct 19, 2022

Looks great overall. I realize I left part of my plan for this feature out of the reporting issue. I want to include support for the NO_COLOR environment variable. See the Output section of the CLIG for more best practices around supporting non-TTY outputs.

Do you want to support only NO_COLOR environment variable or do you want to support other suggestions from Output section of the CLIG (like --no-color)?

Hm. Yea, I'm leaning towards yes. I like that it expands discoverability of that feature. It's a useful enough and generic enough flag that I'm okay bringing that into the CLI contract.

@jirihnidek
Copy link
Contributor Author

Do you want to support only NO_COLOR environment variable or do you want to support other suggestions from Output section of the CLIG (like --no-color)?

Hm. Yea, I'm leaning towards yes. I like that it expands discoverability of that feature. It's a useful enough and generic enough flag that I'm okay bringing that into the CLI contract.

Does it mean that I should add support for --no-color or should I add support for anything else?

@subpop
Copy link
Collaborator

subpop commented Oct 20, 2022

Do you want to support only NO_COLOR environment variable or do you want to support other suggestions from Output section of the CLIG (like --no-color)?

Hm. Yea, I'm leaning towards yes. I like that it expands discoverability of that feature. It's a useful enough and generic enough flag that I'm okay bringing that into the CLI contract.

Does it mean that I should add support for --no-color or should I add support for anything else?

Sorry. Yes. Let's add support for a --no-color CLI flag.

@jirihnidek jirihnidek force-pushed the non-interactive_shell branch 2 times, most recently from 180f0d1 to 0d153d0 Compare October 21, 2022 10:06
Copy link
Collaborator

@subpop subpop left a comment

Choose a reason for hiding this comment

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

The cli package supports getting values for flags from environment variables. Using the NO_COLOR variable as a flag value eliminates some manual checking and consolidates the logic setting the no-color options a bit more.

diff --git a/main.go b/main.go
index 4d852d2..d6c97c0 100644
--- a/main.go
+++ b/main.go
@@ -69,9 +69,10 @@ func main() {
 			Value:  "error",
 		},
 		&cli.BoolFlag{
-			Name:   "no-color",
-			Hidden: false,
-			Value:  false,
+			Name:    "no-color",
+			Hidden:  false,
+			Value:   false,
+			EnvVars: []string{"NO_COLOR"},
 		},
 	}
 	app.Commands = []*cli.Command{
@@ -426,23 +427,11 @@ func main() {
 		}
 		log.SetLevel(level)
 
-		// Detect if the output goes to TTY or some file
-		isColorful = isTerminal(os.Stdout.Fd())
-
-		// When environment variable NO_COLOR is set, then do not display colors and animations too.
-		// We do not care about value of NO_COLOR variable
-		if _, isNoColorSet := os.LookupEnv("NO_COLOR"); isNoColorSet {
-			isColorful = false
-		}
-
-		if c.Bool("no-color") {
-			isColorful = false
-		}
-
-		if !isColorful {
+		if c.Bool("no-color") || !isTerminal(os.Stdout.Fd()) {
 			successPrefix = bwSuccessPrefix
 			failPrefix = bwFailPrefix
 			errorPrefix = bwErrorPrefix
+			isColorful = false
 		}
 
 		return nil

main.go Show resolved Hide resolved
main.go Outdated
isColorful = false
}

if !isColorful {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can become:

Suggested change
if !isColorful {
if c.Bool("no-color") || !isTerminal(os.Stdout.Fd()) {

And you can eliminate lines 429 through 441. Just be sure to set isColorful to false inside the block of code executed when this condition evaluates to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, we can simplify that. BTW: don't you plan to add support for something like --json to status sub-command?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No plan yet, but output that is easily parsed by machines is something I want to add.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

BTW: I updated the PR today morning. Is it OK now?

* When stdout is not TTY, then do not display spinner and status
  text and the output of rhc should not be colorful, because
  output is redirected to some file. When temporary status
  messages and escape sequences were added to log file, then
  viewing such file could be complicated.
* When NO_COLOR environment variable is set, then do not
  display color nor animation too (allowed values are of
  env. var. are: 1, 0, true, false, True, False, TRUE, FALSE).
* When CLI option --no-color is used, then colors and
  animations are suppressed too. Note: this is global
  option and could not be used after sub-command.
* This commit closes RedHatInsights#14 issue
@jirihnidek jirihnidek force-pushed the non-interactive_shell branch from 0d153d0 to a8eea5b Compare October 27, 2022 09:23
Copy link
Collaborator

@subpop subpop left a comment

Choose a reason for hiding this comment

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

Looks great!

@subpop subpop merged commit 5179bf0 into RedHatInsights:main Oct 27, 2022
@jirihnidek jirihnidek deleted the non-interactive_shell branch October 31, 2022 08:21
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.

Better support for non-interactive shell environments
2 participants