Skip to content

adding subid test #8225

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.

Sign up for GitHub

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

Jump to bottom
Open
danlavu wants to merge 1 commit into
base: master
Choose a base branch
from
Open

adding subid test #8225

danlavu wants to merge 1 commit into from
+44 −0

Conversation

@danlavu
Copy link

@danlavu danlavu commented Nov 22, 2025

No description provided.

gemini-code-assist[bot]
gemini-code-assist bot reviewed Nov 22, 2025
View reviewed changes
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds a new system test for subid configuration in IPA. My review focuses on the new test case, where I've found several copy-paste errors in the assertions that would cause the test to fail or report incorrect information. I've provided suggestions to fix the incorrect function calls and assertion messages.

@danlavu danlavu force-pushed the tests-subid branch 2 times, most recently from d731c99 to 64194c5 Compare November 22, 2025 06:54
@danlavu
Copy link
Author

danlavu commented Nov 22, 2025

This requires two other PRs.

jakub-vavra-cz
jakub-vavra-cz approved these changes Nov 24, 2025
View reviewed changes
Copy link
Contributor

@jakub-vavra-cz jakub-vavra-cz left a comment

Choose a reason for hiding this comment

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

LGTM

alexey-tikhonov
alexey-tikhonov reviewed Nov 24, 2025
View reviewed changes
alexey-tikhonov
alexey-tikhonov reviewed Nov 24, 2025
View reviewed changes
alexey-tikhonov
alexey-tikhonov reviewed Nov 24, 2025
View reviewed changes
alexey-tikhonov
alexey-tikhonov reviewed Nov 24, 2025
View reviewed changes
src/tests/system/tests/test_ipa.py Outdated
), f"Subordinate UID start range {uid.range_start} does not match: {subid.uid_start}!"
assert (
subid.uid_size == uid.range_size
), f"Subordinate UID start range {uid.range_size} does not match: {subid.uid_size}!"
Copy link
Member

@alexey-tikhonov alexey-tikhonov Nov 24, 2025

Choose a reason for hiding this comment

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

Copy-paste error.

Copy link
Author

@danlavu danlavu Nov 24, 2025

Choose a reason for hiding this comment

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

It's correct, but I think it's from the confusing variable names and attributes.

subid from ipa has four attributes

  • subid.uid_size
  • subid.uid_range
  • subid.gid_size
  • subid.gid_range

tools.getsubids, returns two

  • range_size
  • range_start

subid.uid_size == object.range_size
subid.uid_start == object.range_start

subid.gid_size == group_object.range_size
subid.gid_start == group_object.range_start

The variables have been changed, let me know if it's still confusing.

Copy link
Member

@alexey-tikhonov alexey-tikhonov Nov 24, 2025

Choose a reason for hiding this comment

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

The text itself - "Subordinate UID start range" - was a copy&paste error.

Copy link
Member

@alexey-tikhonov alexey-tikhonov Nov 24, 2025

Choose a reason for hiding this comment

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

Text says "UID start range" but prints "{uid.range_size}".

Copy link
Author

@danlavu danlavu Dec 1, 2025

Choose a reason for hiding this comment

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

My mistake, fixed.

alexey-tikhonov
alexey-tikhonov reviewed Nov 24, 2025
View reviewed changes
src/tests/system/tests/test_ipa.py Outdated
), f"Subordinate GID start range {gid.range_start} does not match: {subid.gid_start}!"
assert (
subid.gid_size == gid.range_size
), f"Subordinate GID start range {gid.range_size} does not match: {subid.gid_size}!"
Copy link
Member

@alexey-tikhonov alexey-tikhonov Nov 24, 2025

Choose a reason for hiding this comment

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

Copy-paste error.

Copy link
Author

@danlavu danlavu Dec 1, 2025

Choose a reason for hiding this comment

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

Fixed.

alexey-tikhonov
alexey-tikhonov reviewed Nov 24, 2025
View reviewed changes
@danlavu danlavu force-pushed the tests-subid branch 2 times, most recently from d09a147 to 2724d2c Compare December 1, 2025 19:07
@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Dec 2, 2025

@danlavu, mypy fails:

tests/test_ipa.py:671: error: Item "None" of "SubIDEntry | None" has no attribute "range_start"  [union-attr]
tests/test_ipa.py:673: error: Item "None" of "SubIDEntry | None" has no attribute "range_size"  [union-attr]
tests/test_ipa.py:674: error: Item "None" of "SubIDEntry | None" has no attribute "range_size"  [union-attr]
tests/test_ipa.py:679: error: Item "None" of "SubIDEntry | None" has no attribute "range_start"  [union-attr]
tests/test_ipa.py:680: error: Item "None" of "SubIDEntry | None" has no attribute "range_start"  [union-attr]
tests/test_ipa.py:682: error: Item "None" of "SubIDEntry | None" has no attribute "range_size"  [union-attr]
tests/test_ipa.py:683: error: Item "None" of "SubIDEntry | None" has no attribute "range_size"  [union-attr]
Found 8 errors in 1 file (checked 33 source files)

@danlavu danlavu force-pushed the tests-subid branch 2 times, most recently from 8b19216 to ce3167e Compare December 2, 2025 21:29
@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Dec 3, 2025

Note:

tests/test_ipa.py::test_ipa__subids_configured (ipa) PASSED              [ 22%]

@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Dec 3, 2025

@jakub-vavra-cz, are you fine with the latest version?

@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Dec 3, 2025

I set 'Blocked' until patch "do not add this commit" removed.

@alexey-tikhonov
Copy link
Member

alexey-tikhonov commented Dec 3, 2025

SSSD/sssd-test-framework#221 was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexey-tikhonov alexey-tikhonov alexey-tikhonov approved these changes

@jakub-vavra-cz jakub-vavra-cz Awaiting requested review from jakub-vavra-cz

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@alexey-tikhonov alexey-tikhonov

@jakub-vavra-cz jakub-vavra-cz

Labels

backport-to-sssd-2-9-4 Corresponds to C8S backport-to-sssd-2-9 Blocked Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@danlavu @alexey-tikhonov @jakub-vavra-cz