Skip to content

Replacing provider conditionals with set_server method#8081

Merged
alexey-tikhonov merged 1 commit into
SSSD:masterfrom
danlavu:refactor-failover-test
Nov 21, 2025
Merged

Replacing provider conditionals with set_server method#8081
alexey-tikhonov merged 1 commit into
SSSD:masterfrom
danlavu:refactor-failover-test

Conversation

@danlavu

@danlavu danlavu commented Aug 21, 2025

Copy link
Copy Markdown
Contributor

No description provided.

gemini-code-assist[bot]
gemini-code-assist Bot reviewed Aug 21, 2025
View reviewed changes

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

Copy link
Copy Markdown

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 refactors the failover test by replacing a series of provider-specific conditional blocks with a single call to a new set_server method. This is a good simplification of the test code, making it more readable and maintainable. My review includes a minor style fix to adhere to the project's formatting standards.

Comment thread src/tests/system/tests/test_failover.py Outdated
@jakub-vavra-cz jakub-vavra-cz self-assigned this Aug 25, 2025
@jakub-vavra-cz
jakub-vavra-cz self-requested a review August 25, 2025 05:21
jakub-vavra-cz
jakub-vavra-cz requested changes Aug 25, 2025
View reviewed changes

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not really readable, I would expect that this sets correct server and adds a backup one, not that is sets server to invalid.
Maybe something like client.sssd.set_invalid_server() would be better.

@jakub-vavra-cz

jakub-vavra-cz commented Aug 25, 2025

Copy link
Copy Markdown
Contributor

Probably missing branch:sssd-2-9-4.

@ikerexxe
ikerexxe requested a review from thalman August 28, 2025 12:33
thalman
thalman requested changes Aug 29, 2025
View reviewed changes

@thalman thalman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with @jakub-vavra-cz, the naming is misleading.

@danlavu

danlavu commented Aug 31, 2025

Copy link
Copy Markdown
Contributor Author

So, only when failover is set to True.

     def set_server(self, provider: GenericProvider, failover: bool = False) -> None:
         """
         Set the correct 'ldap_server | ipa_server | ad_server' parameter and value for the role.
 
         Optionally sets 'ldap_backup_server | ipa_backup_server | ad_backup_server' for failover testing. When enabled, the working provider server is set to be the backup and a non-existent host is use as the first server.
 
         :param provider: Generic provider object.
         :type provider: GenericProvider
         :param failover: Configure client for failover, defaults to False
         :type failover: bool, optional
         """
         if failover:
             if provider.name == "ldap":
                 self.domain["ldap_uri"] = f"ldap://invalid.{provider.domain}"
                 self.domain["ldap_backup_uri"] = f"ldap://{provider.server}"
             elif provider.name == "ipa":
                 self.domain["ipa_server"] = f"invalid.{provider.domain}"
                 self.domain["ipa_backup_server"] = provider.server
             elif provider.name == "ad":
                 self.domain["ad_server"] = f"invalid.{provider.domain}"
                 self.domain["ad_backup_server"] = provider.server
         else:
             if provider.name == "ldap":
                 self.domain["ldap_uri"] = f"ldap://{provider.server}"
             elif provider.name == "ipa":
                 self.domain["ipa_server"] = provider.server
             elif provider.name == "ad":
                 self.domain["ad_server"] = provider.server

We can change this; the method does work as it's named without that switch. Do you want to split this method into two?

@danlavu

danlavu commented Sep 10, 2025

Copy link
Copy Markdown
Contributor Author

@jakub-vavra-cz What do you want me to do to this method? I all know thumbs up emoji is super descriptive. ;)

@jakub-vavra-cz

jakub-vavra-cz commented Sep 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

@jakub-vavra-cz What do you want me to do to this method? I all know thumbs up emoji is super descriptive. ;)

I would like to see the set server invalid method at least like this:

def set_invalid_server(self, provider: GenericProvider) -> None:

    self.set_server(provider, failover=True)

Posssibly split set_server to two methods.

@danlavu

danlavu commented Sep 11, 2025

Copy link
Copy Markdown
Contributor Author

Ack, I'll split it into two.

@thalman

thalman commented Sep 15, 2025

Copy link
Copy Markdown
Contributor

I changed the label from wait for review to change requested because @danlavu promisesed an update.

@danlavu danlavu mentioned this pull request Sep 15, 2025
Merged
@danlavu
danlavu force-pushed the refactor-failover-test branch from 545fbd2 to 66b648b Compare September 15, 2025 16:57
@danlavu

danlavu commented Sep 15, 2025

Copy link
Copy Markdown
Contributor Author

Waiting on SSSD/sssd-test-framework#204

@danlavu

danlavu commented Sep 16, 2025

Copy link
Copy Markdown
Contributor Author

SSSD/sssd-test-framework#204 is merged, re-running tests.

jakub-vavra-cz
jakub-vavra-cz approved these changes Sep 17, 2025
View reviewed changes

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@jakub-vavra-cz jakub-vavra-cz added the backport-to-sssd-2-9-4 Corresponds to C8S label Sep 17, 2025
@danlavu
danlavu force-pushed the refactor-failover-test branch from 66b648b to 9634a52 Compare September 25, 2025 19:47
thalman
thalman approved these changes Oct 6, 2025
View reviewed changes

@thalman thalman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ACK

@pbrezina
pbrezina force-pushed the master branch 2 times, most recently from f5d64b3 to b854636 Compare November 4, 2025 14:27
@alexey-tikhonov

alexey-tikhonov commented Nov 21, 2025

Copy link
Copy Markdown
Member

@danlavu , please rebase.

@danlavu
danlavu force-pushed the refactor-failover-test branch 2 times, most recently from bbcf737 to 474c788 Compare November 21, 2025 15:47
@danlavu

danlavu commented Nov 21, 2025

Copy link
Copy Markdown
Contributor Author

Done.

@danlavu

danlavu commented Nov 21, 2025

Copy link
Copy Markdown
Contributor Author

The file exists in all the below branches, adding matching tags.

File /src/tests/system/tests/test_failover.py exists in branch sssd-2-10
File /src/tests/system/tests/test_failover.py exists in branch sssd-2-11
File /src/tests/system/tests/test_failover.py exists in branch sssd-2-7
File /src/tests/system/tests/test_failover.py exists in branch sssd-2-8
File /src/tests/system/tests/test_failover.py exists in branch sssd-2-9
File /src/tests/system/tests/test_failover.py exists in branch sssd-2-9-4

Dan Lavu @sssd-bot
Replacing provider conditionals with set_server method
e789b06 
Reviewed-by: Jakub Vávra <[email protected]>
Reviewed-by: Tomáš Halman <[email protected]>
@sssd-bot

sssd-bot commented Nov 21, 2025

Copy link
Copy Markdown
Contributor

The pull request was accepted by @alexey-tikhonov with the following PR CI status:

🟢 CodeFactor (success)
🟢 CodeQL (success)
NEUTRAL osh-diff-scan:fedora-rawhide-x86_64:upstream (neutral)
🟢 rpm-build:centos-stream-10-x86_64:upstream (success)
🟢 rpm-build:fedora-41-x86_64:upstream (success)
🟢 rpm-build:fedora-42-x86_64:upstream (success)
🟢 rpm-build:fedora-43-x86_64:upstream (success)
🟢 rpm-build:fedora-rawhide-x86_64:upstream (success)
🟢 Analyze (target) / cppcheck (success)
🟢 ci / intgcheck (centos-10) (success)
🟢 ci / intgcheck (fedora-41) (success)
🟢 ci / intgcheck (fedora-42) (success)
🟢 ci / intgcheck (fedora-43) (success)
🔴 ci / intgcheck (fedora-44) (failure)
🟢 ci / prepare (success)
🔴 ci / system (centos-10) (failure)
🟢 ci / system (fedora-41) (success)
🟢 ci / system (fedora-42) (success)
🟢 ci / system (fedora-43) (success)
🟢 ci / system (fedora-44) (success)
➖ Coverity scan / coverity (skipped)
🟢 Static code analysis / codeql (success)
🟢 Static code analysis / pre-commit (success)
🟢 Static code analysis / python-system-tests (success)

There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

@sssd-bot
sssd-bot force-pushed the refactor-failover-test branch from 474c788 to e789b06 Compare November 21, 2025 18:07
@alexey-tikhonov
alexey-tikhonov merged commit a9f9c5c into SSSD:master Nov 21, 2025
9 of 14 checks passed
This was referenced Nov 21, 2025
Closed
Merged
Merged
Merged
Merged
@danlavu
danlavu deleted the refactor-failover-test branch January 23, 2026 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thalman thalman thalman approved these changes
@jakub-vavra-cz jakub-vavra-cz jakub-vavra-cz approved these changes
+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

@thalman thalman

@jakub-vavra-cz jakub-vavra-cz

Labels

Accepted backport-to-sssd-2-9-4 Corresponds to C8S backport-to-sssd-2-9 backport-to-sssd-2-10 Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@danlavu @jakub-vavra-cz @thalman @alexey-tikhonov @sssd-bot