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

hosts: Add distro information to BaseLinuxHost #114

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

jakub-vavra-cz
Copy link
Contributor

We need distro information in order to match expected features to distros and versions.

@justin-stephenson
Copy link
Contributor

Would it be better to use https://github.com/python-distro/distro instead?

@alexey-tikhonov
Copy link
Member

Is it expected that 'CI / tox' fail all targets with "docs-upstream: FAIL"?

@jakub-vavra-cz
Copy link
Contributor Author

There is supposedly issue with docs/sphinx.
sphinx-doc/sphinx#12589

@alexey-tikhonov
Copy link
Member

Would it be better to use https://github.com/python-distro/distro instead?

@jakub-vavra-cz, what about this ^^ ?

@jakub-vavra-cz
Copy link
Contributor Author

Would it be better to use https://github.com/python-distro/distro instead?

@jakub-vavra-cz, what about this ^^ ?

@alexey-tikhonov
The library/cli reports about local machine.
I do not want to install this as additional dependency on every machine/container.
I could probably download the release files from the machine, store them somewhere and make the library to parse them but it seems more involved and the code would be probably bigger.

@justin-stephenson
Copy link
Contributor

justin-stephenson commented Jul 25, 2024

Pavel can provide his input on this when he is back but as this OS information is more generic and not SSSD specific, it should more likely be part of pytest-mh. I think it could be added as a new class in https://github.com/next-actions/pytest-mh/tree/master/pytest_mh/utils

Or maybe added to https://github.com/next-actions/pytest-mh/blob/master/pytest_mh/utils/fs.py

@pbrezina
Copy link
Member

pbrezina commented Aug 1, 2024

Pavel can provide his input on this when he is back but as this OS information is more generic and not SSSD specific, it should more likely be part of pytest-mh. I think it could be added as a new class in https://github.com/next-actions/pytest-mh/tree/master/pytest_mh/utils

Hi, this sounds like a good idea.

sssd_test_framework/hosts/base.py Outdated Show resolved Hide resolved
Comment on lines 299 to 302
self._os_release: dict = {}
self._distro_name: str | None = None
self._distro_major: int | None = None
self._distro_minor: int | None = None
Copy link
Member

Choose a reason for hiding this comment

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

It might be nicer to initialize here to some default values and move the call to _distro_information into pytest_setup and remove the @propertyies. It would simplify the code. But I don't insist, whatever you prefer.

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 added the default values.
For most tests we do not need the distro info so I would avoid code that is run for each test uselessly.

Copy link
Member

Choose a reason for hiding this comment

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

It would be run only once, not for each test.

sssd_test_framework/hosts/base.py Outdated Show resolved Hide resolved
sssd_test_framework/hosts/base.py Outdated Show resolved Hide resolved
pbrezina
pbrezina previously approved these changes Sep 12, 2024
@pbrezina
Copy link
Member

Code-wise ack, I did not test it.

@jakub-vavra-cz
Copy link
Contributor Author

@pbrezina
Just tested, the suggested csv oneliner fails me in comparison to my code that actually worked before:

>       self._os_release = dict(csv.reader(os_release.splitlines(), delimiter="="))
E       ValueError: dictionary update sequence element #15 has length 0; 2 is required

The file has an empty line:

[root@client ~]# cat /etc/os-release 
NAME="Red Hat Enterprise Linux"
VERSION="10.0 (Coughlan)"
ID="rhel"
ID_LIKE="fedora"
VERSION_ID="10.0"
PLATFORM_ID="platform:el10"
PRETTY_NAME="Red Hat Enterprise Linux 10.0 Beta (Coughlan)"
ANSI_COLOR="0;31"
LOGO="fedora-logo-icon"
CPE_NAME="cpe:/o:redhat:enterprise_linux:10::baseos"
HOME_URL="https://www.redhat.com/"
VENDOR_NAME="Red Hat"
VENDOR_URL="https://www.redhat.com/"
DOCUMENTATION_URL="https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/10"
BUG_REPORT_URL="https://issues.redhat.com/"

REDHAT_BUGZILLA_PRODUCT="Red Hat Enterprise Linux 10"
REDHAT_BUGZILLA_PRODUCT_VERSION=10.0
REDHAT_SUPPORT_PRODUCT="Red Hat Enterprise Linux"
REDHAT_SUPPORT_PRODUCT_VERSION="10.0 Beta"

@jakub-vavra-cz
Copy link
Contributor Author

Code works now.

pbrezina
pbrezina previously approved these changes Sep 18, 2024
Copy link
Member

@pbrezina pbrezina left a comment

Choose a reason for hiding this comment

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

Code-wise ack. I did not test it, I'll leave it to other reviewers.

If you'd like, you can simplify it more by using @cached_property from functools, you could remove the private properties form the constructor and make distro_information a cached property.

@jakub-vavra-cz
Copy link
Contributor Author

Code-wise ack. I did not test it, I'll leave it to other reviewers.

If you'd like, you can simplify it more by using @cached_property from functools, you could remove the private properties form the constructor and make distro_information a cached property.

I fixed and tested the code and I do not want to do a major refactor again.

@pbrezina
Copy link
Member

Why did you unassigned @justin-stephenson and @alexey-tikhonov?

@pbrezina pbrezina merged commit d622b53 into SSSD:master Sep 24, 2024
5 checks passed
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.

4 participants