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

Allow user to force terminal size used by the CLI formatters by setting environment variable #4242

Merged
merged 13 commits into from
Jul 13, 2018

Conversation

Kami
Copy link
Member

@Kami Kami commented Jul 13, 2018

Right now, we try various approaches to obtain terminal size used by the CLI for table formatting purposes.

If none of that approaches work, we fall back to a default hard coded size (10 lines, 20 columns).

This pull request improves on that in two ways:

  1. It uses a more reasonable default size. 20 columns for a total terminal width is simply a bad default. It means pretty much every table cell / column will wrap at couple of characters which will make output unreadable when we are unable to retrieve a terminal size.
  2. Allows user to force terminal size using ST2_CLI_FORCE_TERMINAL_SIZE=<lines,columns> environment variable.

We have experienced this "issue" a lot in our end to end tests for a long time. Since those test commands don't run using shell, we fail to obtain a terminal size and we fall back to a really bad default which results in an unreadable output like that:

Before:

screenshot from 2018-07-13 14-07-45

After:

screenshot from 2018-07-13 14-08-06

Keep in mind that those actions run on a remote host which is later destroyed so we can't simply re-retrieve the output which means we are stuck with troubleshooting / debugging from this very hard to read output.

TODO

  • Changelog entry

@Kami Kami added this to the 2.9.0 milestone Jul 13, 2018
"""
:return: (lines, cols)
"""
# Allow user to force terminal size using a environment variables
# E.g. ST2_CLI_FORCE_TERMINAL_SIZE=80,200 # lines, columns
Copy link
Member

@arm4b arm4b Jul 13, 2018

Choose a reason for hiding this comment

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

+1 for changing the default limits, but to force the terminal size we can rely on existing $COLUMNS and $LINES, without implementing new ST2_CLI_FORCE_TERMINAL_SIZE:

# try environment variables
try:
return tuple(int(os.getenv(var)) for var in ('LINES', 'COLUMNS'))
except:
pass

Copy link
Member Author

@Kami Kami Jul 13, 2018

Choose a reason for hiding this comment

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

The problem is that this will only force it in case we fall back through all the other approaches to this very last one - aka it will only work if we can't retrieve the terminal size using other approaches.

User won't be able to force a different size in case we are able to retrieve the size using some other approach.

So that would be a different thing, more like ST2_CLI_FALLBACK_TERMINAL_SIZE or similar. But with the current approach, it can be overridden in any scenario since that environment variable has precedence over other approaches.

Copy link
Member

Choose a reason for hiding this comment

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

So ideally if st2 would rely on those vars.

For example, something like was typical while working with Docker:

kubectl exec -it ... env COLUMNS=80 LINES=200 TERM=xterm /bin/bash

Copy link
Member Author

@Kami Kami Jul 13, 2018

Choose a reason for hiding this comment

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

Yes, it will still rely on those two environment variables if other method fails, but with this new environment variable there is also a way to override those values (e.g. in case some other method return a value, but user wants to use a different one without changing xterm settings, etc.).

variables first.

This way it's more performant and consistent with upstream Python 3
implementation -
https://github.com/python/cpython/blob/master/Lib/shutil.py#L1203.

This way there is also no need for additional environment variables
since those values are checked first.
@Kami
Copy link
Member Author

Kami commented Jul 13, 2018

@Arma and I discussed this some more and decided to go with this approach - 471b5ef.

Now we check LINES and COLUMNS environment variables first so there is no need for additional environment variables.

This way it's also consistent with upstream Python 3 implementation of get_terminal_size (https://github.com/python/cpython/blob/master/Lib/shutil.py#L1203).

Keep in mind that we need to maintain our own implementation because this function is not available in Python 2.

# checked those variables at the end as a fall back.
try:
lines = os.environ['LINES']
columns = os.environ['COLUMNS']
Copy link
Member

@arm4b arm4b Jul 13, 2018

Choose a reason for hiding this comment

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

👍 on approach, but I think original implementation is safer on corner cases here:
https://github.com/python/cpython/blob/master/Lib/shutil.py#L1223-L1231

For example, when only LINES or only COLUMNS can be overridden by the user.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, agreed - our implementation requires both variables to be set (but we actually only use columns and ignore lines all together).

Original implementation allows you to specify those independently. I was thinking of making that change as well, but it's a lot more involved than just doing what I do right now (and previous code also requires both to be specified).

Copy link
Member

Choose a reason for hiding this comment

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

Setting only one var and expect it to take precedence is a valid behavior with LINES vs COLUMNS. It would be a surprise that only 2 alltogether would work.

We're following the established standards here. Ideally if we won't surprise user in a bad way :)

We can use a waterfall of fallbacks as in original .py implementation:
https://github.com/python/cpython/blob/master/Lib/shutil.py#L1233-L1234
https://github.com/python/cpython/blob/master/Lib/shutil.py#L1241-L1244

If we change the implementation to read columns only, get_terminal_size function wouldn't do what it's advertised/expected to do.

Kami added 4 commits July 13, 2018 17:01
We only use columns value of terminal size so this allows us to simplify
code for retrieving terminal size.
import os
import struct
import subprocess
import sys

from st2client.utils.color import format_status

DEFAULT_TERMINAL_SIZE_LINES = 80
Copy link
Member

@arm4b arm4b Jul 13, 2018

Choose a reason for hiding this comment

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

Can remove as never needed/used

@Kami Kami merged commit a800b30 into master Jul 13, 2018
@Kami Kami deleted the st2_client_default_column_width branch July 13, 2018 17:52
@warrenvw
Copy link
Contributor

Thanks. I guess we won't need StackStorm/st2-docker#102 anymore... 👍

@Kami Kami mentioned this pull request Jul 16, 2018
@Kami Kami modified the milestones: 2.9.0, 2.8.1 Jul 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants