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

"lando ssh" doesn't respect TERM, clobbering it with TERM=xterm #2827

Open
phil-s opened this issue Jan 28, 2021 · 12 comments
Open

"lando ssh" doesn't respect TERM, clobbering it with TERM=xterm #2827

phil-s opened this issue Jan 28, 2021 · 12 comments
Labels
bug Something aint working right!

Comments

@phil-s
Copy link

phil-s commented Jan 28, 2021

Tell us about your setup

v3.0.24 on Ubuntu 18.04 GNU/Linux

Tell us about your .lando.yml

name: lamp2
recipe: lamp

No other config present for this test.

Tell us about the command you were running

$ lando start # using above .lando.yml

$ echo $TERM
eterm-color

$ lando ssh
www-data@9df61e345dfb:/app$ echo $TERM
xterm

Tell us generally about your bug

I think this might be coming from line 18 here:

// Default env values
const defaults = {
config: {
appEnv: {
COLUMNS: 256,
LANDO: 'ON',
LANDO_WEBROOT_USER: 'www-data',
LANDO_WEBROOT_GROUP: 'www-data',
TERM: 'xterm',
},
appLabels: {
'io.lando.container': 'TRUE',
},
},
};

I've noted that in my ~/.lando/compose/PROJECT/globals-0.yml file I have that same sequence of environment vars set for every single container. E.g.:

services:
  appserver:
    networks:
      default: {}
    environment:
      COLUMNS: 256
      LANDO: 'ON'
      LANDO_WEBROOT_USER: www-data
      LANDO_WEBROOT_GROUP: www-data
      TERM: xterm
      [...]

I tried editing this file as a test, but it gets automatically re-written with that same TERM: xterm value; so that's obviously being pulled from somewhere else, but I don't know where -- I don't have that plugins/lando-core/index.js file on my system, so I guess that gets compiled into something else. In any case, it meant I wasn't able to confirm whether or not this was actually the culprit; but it certainly looks like it could be.

Tell us more

It's important to respect the user's exported TERM variable, as any command which is run inside the container and produces output potentially needs to know your terminal in order to produce sane output.

As it is, the output produced will always be for an xterm, and therefore potentially broken for the terminal it is actually being displayed in.

@phil-s phil-s added the bug Something aint working right! label Jan 28, 2021
@phil-s
Copy link
Author

phil-s commented Jan 28, 2021

I'll also just confirm that in the above test with recipe: lamp the ncurses-term debian package is installed as standard, which means that my eterm-color terminal is supported out of the box (/usr/share/terminfo/e/); so my issue isn't the result of some kind of fallback behaviour for an unrecognised terminal (although such behaviour might be a good idea).

@pirog
Copy link
Member

pirog commented Feb 2, 2021

@phil-s im not sure why we explicitly set TERM: xterm but im guessing its vestigial and can be removed. That said i think under the hood Docker is also going to just set this to xterm but i'd have to double check on that.

Lando does let you override envvars so that might work but i havent tried it with TERM explicitly.

@phil-s
Copy link
Author

phil-s commented Feb 2, 2021

Lando does let you override envvars so that might work but i havent tried it with TERM explicitly.

I may have tested that myself. I experimented with adding TERM=eterm-color in one of the files which we've registered with env_file: in .lando.yml, and I did a lando rebuild, but the new setting didn't seem to get used.

This wouldn't be an ideal solution in any case, as it would still be hard-coding a specific terminal, and there's no guarantee that someone is always using the same terminal. Even discounting those of us who have reason to use more than one on a single machine (I use two different types daily), a person might easily have a different terminal type when connecting from work vs connecting from home; or desktop vs laptop, etc; so it's still pretty easy to end up with a mismatch.

I wouldn't know where to look regarding docker's own behaviours -- I'm fairly new to both lando and docker -- but if you find that it's really a docker issue, I'll happily take this upstream.

@phil-s
Copy link
Author

phil-s commented Feb 2, 2021

This recent docker issue seems relevant: docker/cli#2938

If I've understood correctly, it also suggests that lando might be able to "add -e TERM=${TERM} to the command line arguments" when it calls docker? That could be a useful test at minimum; and if there's an existing way for users to configure the docker options which will be used(?), then it could be an immediate workaround. (My search terms for this proved a bit broad, so I'm not sure whether lando provides for that or not.)

@pirog
Copy link
Member

pirog commented Feb 2, 2021

@phil-s we allow per-user override files for situations like that:
https://docs.lando.dev/config/lando.html#override-file

You can also set envvars on a per-service and per-command basis:

name: lando-tooling
service:
  node:
    type: node10
    overrides:
      environment:
        TERM: xterm-256
tooling:
  node:
    service: node
  yarn: 
    service: node
    env:
      TERM: my-term

To get back to the issue even if we remove our TERM: xterm it looks like Docker still sets that to xterm so in either case you would need to manually set the term yourself using one of the available ways to do so in Lando.

@phil-s
Copy link
Author

phil-s commented Feb 3, 2021

Thanks for that; I tried the overrides approach, and it worked! It's still a hard-coded value, and it looks like it needs a lando rebuild to pick up a change, but it's good to know that we can easily change what the hard-coded value is.

To get back to the issue even if we remove our TERM: xterm it looks like Docker still sets that to xterm so in either case you would need to manually set the term yourself using one of the available ways to do so in Lando.

That "add -e TERM=${TERM} to the command line arguments" suggestion from the related issue sounded quite promising as a way to dynamically pass through the correct TERM value, but I presume that lando itself would need to do that when it calls docker? I don't know if there's anything I can currently do or configure to cause lando to use that docker option?

@stale
Copy link

stale bot commented Aug 3, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions and please check out this if you are wondering why we auto close issues.

@stale stale bot added the stale Issue has been auto-flagged as stale label Aug 3, 2021
@phil-s
Copy link
Author

phil-s commented Aug 3, 2021

"further activity".

Making lando pass -e TERM=${TERM} still sounds like a viable solution.

@stale stale bot removed the stale Issue has been auto-flagged as stale label Aug 3, 2021
@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions and please check out this if you are wondering why we auto close issues.

@stale stale bot added the stale Issue has been auto-flagged as stale label Apr 16, 2022
@phil-s
Copy link
Author

phil-s commented Apr 16, 2022

"further activity".

Making lando pass -e TERM=${TERM} still sounds like a viable solution.

@stale stale bot removed the stale Issue has been auto-flagged as stale label Apr 16, 2022
@stale
Copy link

stale bot commented May 22, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions and please check out this if you are wondering why we auto close issues.

@stale stale bot added the stale Issue has been auto-flagged as stale label May 22, 2023
@phil-s
Copy link
Author

phil-s commented May 23, 2023

"further activity".

Making lando pass -e TERM=${TERM} still sounds like a viable solution.

@stale stale bot removed the stale Issue has been auto-flagged as stale label May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something aint working right!
Projects
None yet
Development

No branches or pull requests

2 participants