uyuni-project / uyuni-tools

Tools to work with containerized Uyuni server
Apache License 2.0
14 stars 18 forks source link

Preserve PAGER settings from the host #393

Closed aaannz closed 2 months ago

aaannz commented 2 months ago

What does this PR change?

This fixes pager in mgradm support sql -i, however I am not 100% sure if this is correct as is. On one hand, respecting host PAGER should be valid approach, but if someone set custom one which we don't have on the container image this will cause annoying problems. On the other hand, if I let use setting from the container, that might be different from what people have in their setups and furthermore override will not work anymore.

Test coverage

Links

Issue(s): https://github.com/SUSE/spacewalk/issues/24649

Changelogs

Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository

If you don't need a changelog check, please mark this checkbox:

If you uncheck the checkbox after the PR is created, you will need to re-run changelog_test (see below)

Before you merge

Check How to branch and merge properly!

mcalmer commented 2 months ago

Even with uyuni-server:/ # export PAGER=less uyuni-server:/ # export LESS="-M -I -R"

there is some problems with umlauts

 id  |                 name                  |                                     description                                     | current_members | group_type | org_id |            created            |     >
-----+---------------------------------------+-------------------------------------------------------------------------------------+-----------------+------------+--------+-------------------------------+----->
  67 | hall<C3><B6>le                              | group with umlaut <C3><B6>                                                                |               0 |            |      1 | 2013-06-17 09:07:02+00  >
  68 | group<C3><A9>                               | group with <C3><A9> \xe9                                                                  |               0 |            |      1 | 2013-06-17 09:19:27+00  >
 id |   name   |     description      | current_members | group_type | org_id |        created         |           modified            
----+----------+----------------------+-----------------+------------+--------+------------------------+-------------------------------
 67 | hallöle | group with umlaut ö |               0 |            |      1 | 2013-06-17 09:07:02+00 | 2022-05-25 08:52:01.182574+00
aaannz commented 2 months ago

I tried to recreate that group and I see it correctly. How can I reproduce that umlaut issue?

@cbosdo do we also want this in the mgrctl term? I guess we do. And mgrctl exec in interactive mode too I expect.

aaannz commented 2 months ago

I redid it a little and now when we work interactively (pseudo-tty is allocated) we pass more envvars from host. Inspired by what /etc/profile does.

When people will want to use our tools in scripts, e.g. echo $data | mgrctl exec -i $command | grep $something, then none of these vars are passed in. I wonder now if we should also move ENV=/etc/sh.shrc.local to be used only with -t? Because when only -i is used, it will not source that anyway.

cbosdo commented 2 months ago

I tried to recreate that group and I see it correctly. How can I reproduce that umlaut issue?

@cbosdo do we also want this in the mgrctl term? I guess we do. And mgrctl exec in interactive mode too I expect.

We probably need it for mgrctl exec interactive mode. For mgrctl term it's different as you get a terminal inside the container. I'm not sure it would make sense to use host variables in it.

aaannz commented 2 months ago

Current implementation of term just appends -i -t for the exec command. So what we add for them is inherited by term. I would also argue that precisely because term gets terminal inside container, it should follow what user has on it's host system which is also modified by ssh, etc.

cbosdo commented 2 months ago

Current implementation of term just appends -i -t for the exec command. So what we add for them is inherited by term. I would also argue that precisely because term gets terminal inside container, it should follow what user has on it's host system which is also modified by ssh, etc.

Let's keep it as is. No need to over engineer it. We can still think about it later if someone complains