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

Fix ST2 Client for Windows Clients #6071

Merged
merged 12 commits into from
Nov 28, 2023
Merged

Conversation

philipphomberger
Copy link

We have try to rollout the st2client on the Windows 10/11 clients of our StackStorm users. But because PWD only exist on Unix Based Operation System it failed.

@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Nov 26, 2023
CHANGELOG.rst Outdated Show resolved Hide resolved
@philipphomberger philipphomberger marked this pull request as ready for review November 26, 2023 15:54
@arm4b
Copy link
Member

arm4b commented Nov 26, 2023

For the record, what's the error you're getting on Windows systems?

@philipphomberger
Copy link
Author

philipphomberger commented Nov 26, 2023

For the record, what's the error you're getting on Windows systems?
Screenshot:

image

Text Format:
(venv2) PS C:\Users\philipphomberger\venv2\Scripts> st2 Traceback (most recent call last): File "C:\Users\philipphomberger\AppData\Local\Programs\Python\Python38\lib\runpy.py", line 192, in _run_module_as_main return _run_code(code, main_globals, None, File "C:\Users\philipphomberger\AppData\Local\Programs\Python\Python38\lib\runpy.py", line 85, in _run_code exec(code, run_globals) File "C:\Users\philipphomberger\venv2\Scripts\st2.exe\__main__.py", line 4, in <module> File "c:\users\philipphomberger\venv2\lib\site-packages\st2client\shell.py", line 43, in <module> from st2client.base import BaseCLIApp File "c:\users\philipphomberger\venv2\lib\site-packages\st2client\base.py", line 48, in <module> import pwd ModuleNotFoundError: No module named 'pwd'

@nzlosh
Copy link
Contributor

nzlosh commented Nov 27, 2023

Based on the proposed fix os.getlogin() returns the correct information on Windows. I don't know the context around the patch, but the workaround for getlogin was added 11/07/2017 so I question it's relevance. I checked on the currently supported OS' and a couple of others. They all function as expected, so I'd be inclined to use the standard os.getlogin() function to have cross-platform support provided by Python, rather than maintaining these exceptions in the StackStorm code base.

CentOS Linux 8

>>> sys.version
'3.6.8 (default, Sep 10 2021, 09:13:53) \n[GCC 8.5.0 20210514 (Red Hat 8.5.0-3)]'

>>> os.getlogin()
'root'

Ubuntu 20.04.6 LTS

>>> sys.version
'3.8.10 (default, May 26 2023, 14:05:08) \n[GCC 9.4.0]'

>>> os.getlogin()
'root'

Debian GNU/Linux 10 (buster)

>>> sys.version
'3.7.3 (default, Jun 29 2023, 18:03:57) \n[GCC 8.3.0]'

>>> os.getlogin()
'root'

Rocky Linux 9.2 (Blue Onyx)

>>> sys.version
'3.9.16 (main, May 29 2023, 00:00:00) \n[GCC 11.3.1 20221121 (Red Hat 11.3.1-4)]'

>>> os.getlogin()
'root'

Ubuntu 18.04.6 LTS

>>> sys.version
'3.6.9 (default, Mar 10 2023, 16:46:00) \n[GCC 8.4.0]'

>>> os.getlogin()
'root'

Ubuntu 22.04.3 LTS

>>> sys.version
'3.10.12 (main, Jun 11 2023, 05:26:28) [GCC 11.4.0]'

>>> os.getlogin()
'root'

@philipphomberger
Copy link
Author

Hi Carlos, thank you for your Feedback.
Unfortunately, I also don't know why exactly the original fix was installed.
I can only comprehend the Problem in my Github Codespace Linux.
grafik
But I think this is a Microsoft special Linux.
Since the problem is in the supported versions, it would be better to use the integrated Python function as you said instead of having the special lock in the code.

@nzlosh
Copy link
Contributor

nzlosh commented Nov 27, 2023

I suspect you might be hitting this issue with your environment python/cpython#84998. As per the recommendation in the issue, perhaps getpass.getuser() is a more robust solution? Does it work on windows 10/11?

Tested with other OS's, getuser works as expected:

VERSION="18.04.6 LTS (Bionic Beaver)"
python3 -c import getpass; print(getpass.getuser())
root
VERSION="20.04.6 LTS (Focal Fossa)"
python3 -c import getpass; print(getpass.getuser())
root
VERSION="22.04.3 LTS (Jammy Jellyfish)"
python3 -c import getpass; print(getpass.getuser())
root
VERSION="8"
python3 -c import getpass; print(getpass.getuser())
root
VERSION="9.2 (Blue Onyx)"
python3 -c import getpass; print(getpass.getuser())
root
VERSION="10 (buster)"
python3 -c import getpass; print(getpass.getuser())
root

st2client/st2client/base.py Outdated Show resolved Hide resolved
# https://docs.python.org/3.8/library/pwd.html
# Windows Default ENVVARS -> https://www.computerhope.com/issues/ch000088.htm
if platform.system() == "Windows":
os.getlogin = lambda: os.environ.get("USERNAME")
Copy link
Member

@arm4b arm4b Nov 26, 2023

Choose a reason for hiding this comment

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

As discussed, it's worth checking if the default os.getlogin() works under Windows.
@philipphomberger could you try that?
Something to try before relying on a custom implementation.

If so, we could leave the old workaround conditional for non-windows systems and let Windows rely on native os.getlogin().

Copy link
Author

Choose a reason for hiding this comment

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

I have test it on Windows looks working fine.
image
I'll adjust it right away.

@arm4b arm4b added the bug label Nov 27, 2023
@arm4b arm4b added this to the 3.8.1 milestone Nov 27, 2023
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Looks good,
Thanks for the fix!

@arm4b arm4b enabled auto-merge November 27, 2023 20:59
@arm4b arm4b merged commit d54abcf into StackStorm:master Nov 28, 2023
35 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug size/S PR that changes 10-29 lines. Very easy to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants