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.

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

Tests: ldap search base does not fully limit the Netgroup search base #7754

Closed
wants to merge 1 commit into from
Closed

Tests: ldap search base does not fully limit the Netgroup search base #7754

wants to merge 1 commit into from

Conversation

aborah-sudo
Copy link
Contributor

ldap search base does not fully limit the Netgroup search base

jakub-vavra-cz
jakub-vavra-cz requested changes Dec 10, 2024
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.

  1. see inline comments
  2. Should this be backported to sssd-2-10 or/and other branches?

src/tests/system/tests/test_ldap.py Outdated Show resolved Hide resolved
src/tests/system/tests/test_ldap.py Outdated Show resolved Hide resolved
src/tests/system/tests/test_ldap.py Outdated Show resolved Hide resolved
src/tests/system/tests/test_ldap.py Outdated Show resolved Hide resolved
src/tests/system/tests/test_ldap.py Outdated Show resolved Hide resolved
@aborah-sudo aborah-sudo force-pushed the ldapp branch 3 times, most recently from 315c884 to 7b0ca0a Compare December 11, 2024 03:53
ikerexxe
ikerexxe reviewed Dec 12, 2024
View reviewed changes
Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

I had a set of inline comments prepared for this test case, but the more I went through it the less I understood, so I decided to remove them and ask the following question: What are you trying to test? Can you specify it in the form of a use case?

@aborah-sudo
Copy link
Contributor Author

I had a set of inline comments prepared for this test case, but the more I went through it the less I understood, so I decided to remove them and ask the following question: What are you trying to test? Can you specify it in the form of a use case?

Here sssd-ldap option "ldap_search_base" is being tested . Here in this test "ou=Netgroup1,dc=ldap,dc=test" is set as ldap_search_base and ldap_search should limit to this OU only

@aborah-sudo aborah-sudo force-pushed the ldapp branch 2 times, most recently from da07476 to 7cb4165 Compare December 12, 2024 09:11
@aborah-sudo aborah-sudo requested a review from ikerexxe December 12, 2024 09:11
@aborah-sudo aborah-sudo force-pushed the ldapp branch 7 times, most recently from f48140d to 0309a99 Compare December 17, 2024 09:08
jakub-vavra-cz
jakub-vavra-cz requested changes Jan 17, 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.

See inline.

@aborah-sudo aborah-sudo added the branch: sssd-2-9-4 Corresponds to C8S label Jan 17, 2025
jakub-vavra-cz
jakub-vavra-cz approved these changes Jan 17, 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.

ikerexxe
ikerexxe approved these changes Jan 17, 2025
View reviewed changes
Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

LGTM!

@aborah-sudo aborah-sudo added the Ready to push Ready to push label Jan 17, 2025
@alexey-tikhonov alexey-tikhonov added no-backport This should go to target branch only. Ready to push Ready to push Blocked and removed Ready to push Ready to push branch: sssd-2-8 branch: sssd-2-9 branch: sssd-2-10 branch: sssd-2-9-4 Corresponds to C8S labels Jan 17, 2025
@alexey-tikhonov
Copy link
Member

This patch doesn't apply even to sssd-2-10 branch, and this is definitely wrong.

A brief inspections shows that at least 6040510 and 7b855ab are missing in sssd-2-10 branch (maybe more).

This is most probably because of missed backport-to- labels on previous PRs.

@aborah-sudo, @jakub-vavra-cz, @kaushikub, please make sure sssd-2-10 (and older branches if needed) do not miss any patches present in 'master' branch.

I set this PR as "blocked" until issue is resolved to avoid making discrepancy worse.

@aborah-sudo
Copy link
Contributor Author

aborah-sudo commented Jan 18, 2025 •
edited
Loading

This patch doesn't apply even to sssd-2-10 branch, and this is definitely wrong.

A brief inspections shows that at least 6040510 and 7b855ab are missing in sssd-2-10 branch (maybe more).

This is most probably because of missed backport-to- labels on previous PRs.

@aborah-sudo, @jakub-vavra-cz, @kaushikub, please make sure sssd-2-10 (and older branches if needed) do not miss any patches present in 'master' branch.

I set this PR as "blocked" until issue is resolved to avoid making discrepancy worse.

#7805 is now created to backport the last prs to 2.10

@alexey-tikhonov
Copy link
Member

Pushed PR: #7754

  • master
    • e76849b - Tests: ldap search base does not fully limit the Netgroup search base
  • sssd-2-10
    • bdeae92 - Tests: ldap search base does not fully limit the Netgroup search base

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

@jakub-vavra-cz jakub-vavra-cz jakub-vavra-cz approved these changes

@ikerexxe ikerexxe ikerexxe approved these changes

Assignees

@ikerexxe ikerexxe

@jakub-vavra-cz jakub-vavra-cz

Labels
branch: sssd-2-10 Pushed Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aborah-sudo @alexey-tikhonov @ikerexxe @jakub-vavra-cz