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

Issue 6483 - CI - fix filter_test - test_match_large_valueset #6484

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

mreynolds389
Copy link
Contributor

Description:

Fix filter_test::test_match_large_valueset

The suffix parameter to dbgen_users was using the wrong DN (user ou versus DEFAULT suffix)

Relates: #6483

Copy link
Member

@droideck droideck left a comment

Choose a reason for hiding this comment

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

Besides the minor concern, - Looks good!

Comment on lines 376 to 377
except Exception as e:
raise e
Copy link
Member

Choose a reason for hiding this comment

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

I think this part is completely redundant...
It basically just catches and immediately re-raises the same exception without performing any additional actions.

Even though it's better than bare except: raise - still, I'd just remove it:)

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 was just fixing lint errors, I can take it out.

Copy link
Contributor

@tbordaz tbordaz left a comment

Choose a reason for hiding this comment

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

LGTM

Description:

Fix filter_test::test_match_large_valueset

The suffix and parent parameters to dbgen_users was switched

Relates: 389ds#6483

Reviewed by: spichugi(Thanks!)
@mreynolds389 mreynolds389 merged commit 6935548 into 389ds:main Jan 9, 2025
9 checks passed
@mreynolds389 mreynolds389 deleted the fix_dbgen branch January 9, 2025 13:39
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.

3 participants