Gdm c9s updates by spoore1 · Pull Request #250 · SSSD/sssd-test-framework

spoore1 · 2026-05-27T19:52:59Z

gdm: fixes to better support c9s/rhel9

Because the log messages for GDM/Gnome differ between c9s versions and
newer, the wait_for_login method in the gdm util now checks for two
different log messages to confirm login before continuing.

topologies: hostname and setup/teardown fixes

topology_controller updates for hostname changes and setup/teardown
stability.

Keycloak tests when run manually often didn't clean up correctly
(even with backup/restores) so the setup is updated to remove clients
before trying to add if found.

GDM topology controller didn't have some necessary updates to make
hostname changes work during setup.

topology_controller updates for hostname changes and setup/teardown stability. Keycloak tests when run manually often didn't clean up correctly (even with backup/restores) so the setup is updated to remove clients before trying to add if found. GDM topology controller didn't have some necessary updates to make hostname changes work during setup.

Because the log messages for GDM/Gnome differ between c9s versions and newer, the wait_for_login method in the gdm util now checks for two different log messages to confirm login before continuing.

gemini-code-assist

Code Review

This pull request introduces several improvements to the SSSD test framework, including robust error handling during domain uninstallation, hostname configuration backups, and cleanup of existing Keycloak clients and IPA IDP entries before recreation. In sssd_test_framework/utils/gdm.py, the login detection mechanism is updated to check for multiple patterns. The review feedback suggests refactoring duplicated domain uninstallation and hostname setup logic, combining regex patterns in wait_for_login for better readability, and removing a redundant inline import time statement.

gemini-code-assist · 2026-05-27T19:55:45Z

            if isinstance(provider, (IPAHost)):
-                client.conn.exec(["ipa-client-install", "--uninstall", "-U"])
+                result = client.conn.exec(["ipa-client-install", "--uninstall", "-U"], raise_on_error=False)
+                if result.rc != 0:
+                    self.logger.info(
+                        f"Running ipa-client-install --uninstall failed with:\n{result.stdout}\n{result.stderr}"
+                    )
+                    self.logger.info("Trying to remove sssd.conf now")
+                    client.fs.rm("/etc/sssd/sssd.conf")
            else:
-                client.conn.exec(["realm", "leave", "--unattended", provider.domain], input=provider.adminpw)
+                result = client.conn.exec(
+                    ["realm", "leave", "--unattended", provider.domain], input=provider.adminpw, raise_on_error=False
+                )
+                if result.rc != 0:
+                    self.logger.info(f"Running realm leave failed with:\n{result.stdout}\n{result.stderr}")
+                    self.logger.info("Trying to remove sssd.conf now")
+                    client.fs.rm("/etc/sssd/sssd.conf")


The error handling logic for ipa-client-install --uninstall and realm leave is duplicated. To improve maintainability and reduce redundancy, this can be refactored into a single block of code.

if isinstance(provider, (IPAHost)): uninstall_cmd = ["ipa-client-install", "--uninstall", "-U"] uninstall_input = None cmd_desc = "ipa-client-install --uninstall" else: uninstall_cmd = ["realm", "leave", "--unattended", provider.domain] uninstall_input = provider.adminpw cmd_desc = "realm leave" result = client.conn.exec(uninstall_cmd, input=uninstall_input, raise_on_error=False) if result.rc != 0: self.logger.info(f"Running {cmd_desc} failed with:\n{result.stdout}\n{result.stderr}") self.logger.info("Trying to remove sssd.conf now") client.fs.rm("/etc/sssd/sssd.conf")

gemini-code-assist · 2026-05-27T19:55:45Z

        short_hostname = client.conn.run("hostname").stdout.split(".")[0].strip()
-        hostname = f"{short_hostname}.{keycloak.domain}"
+        hostname = f"{short_hostname}.{ipa.domain}"
+        client.fs.backup("/etc/hostname")
+        client.fs.backup("/etc/hosts")
+        client.conn.run(f"echo {hostname} > /etc/hostname")
+        client.fs.write("/etc/hosts", client.fs.read("/etc/hosts").replace("client.test", hostname))


This block of code for setting the client hostname is also present in IPATopologyController. To improve maintainability and avoid code duplication, consider extracting this logic into a private helper method, e.g., _set_client_hostname(self, client: ClientHost, domain: str).

gemini-code-assist · 2026-05-27T19:55:45Z

+        rc = 0
+        checks = ["Opening and taking control of.*card", "Adding device.*card"]
+        for check in checks:
+            if not re.search(check, result.stdout):
+                rc += 1
+
+        if rc >= len(checks):
            raise AssertionError("Unable to see gnome-shell take control of video card")


Instead of iterating through the checks and calling re.search in a loop, it's more efficient and readable to combine the patterns into a single regular expression using |.

checks = ["Opening and taking control of.*card", "Adding device.*card"] pattern = "|".join(checks) if not re.search(pattern, result.stdout): raise AssertionError("Unable to see gnome-shell take control of video card")

This one makes sense

gemini-code-assist · 2026-05-27T19:55:45Z

+
+        if rc >= len(checks):
            raise AssertionError("Unable to see gnome-shell take control of video card")
+        import time


The time module is already imported at the top of this file. This import statement is redundant and violates PEP 8 style guidelines, which recommend placing all imports at the top of the file. Please remove this line.

ikerexxe

Some minor issues inline. In addition, I'd highly recommend to read gemini's proposals

ikerexxe · 2026-05-28T13:18:54Z

+        rc = 0
+        checks = ["Opening and taking control of.*card", "Adding device.*card"]
+        for check in checks:
+            if not re.search(check, result.stdout):
+                rc += 1
+
+        if rc >= len(checks):
            raise AssertionError("Unable to see gnome-shell take control of video card")


This one makes sense

ikerexxe · 2026-05-28T13:21:43Z

+                    self.logger.info(
+                        f"Running ipa-client-install --uninstall failed with:\n{result.stdout}\n{result.stderr}"
+                    )
+                    self.logger.info("Trying to remove sssd.conf now")


I'd state Removing sssd.conf now, unless a check for the removal of the file exists somewhere

ikerexxe · 2026-05-28T13:22:05Z

+                )
+                if result.rc != 0:
+                    self.logger.info(f"Running realm leave failed with:\n{result.stdout}\n{result.stderr}")
+                    self.logger.info("Trying to remove sssd.conf now")


ikerexxe · 2026-05-28T13:23:43Z

+            self.logger.info(f"IPA already enrolled in keycloak.  Removing.")
+            ipa.conn.run("ipa idp-del keycloak")
+
+        result = ipa.conn.run(


Either you check for something on result or you don't assign any value here

ikerexxe · 2026-05-28T13:42:02Z

+        # Check if IPA entry for Keycloak already exists and delete
+        result = ipa.conn.run("ipa idp-show keycloak", raise_on_error=False)
+        if "ipa_oidc_client" in result.stdout:
+            self.logger.info(f"IPA already enrolled in keycloak.  Removing.")


This doesn't need to be an f-string

spoore1 added 2 commits May 27, 2026 14:51

gdm: fixes to better support c9s/rhel9

581735c

Because the log messages for GDM/Gnome differ between c9s versions and newer, the wait_for_login method in the gdm util now checks for two different log messages to confirm login before continuing.

gemini-code-assist Bot reviewed May 27, 2026

View reviewed changes

alexey-tikhonov requested a review from ikerexxe May 28, 2026 12:42

alexey-tikhonov assigned ikerexxe May 28, 2026

ikerexxe reviewed May 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gdm c9s updates#250

Gdm c9s updates#250
spoore1 wants to merge 2 commits into
SSSD:masterfrom
spoore1:gdm_c9s_updates

spoore1 commented May 27, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot May 27, 2026

Uh oh!

gemini-code-assist Bot May 27, 2026

Uh oh!

gemini-code-assist Bot May 27, 2026

Uh oh!

ikerexxe May 28, 2026

Uh oh!

gemini-code-assist Bot May 27, 2026

Uh oh!

ikerexxe left a comment

Uh oh!

ikerexxe May 28, 2026

Uh oh!

ikerexxe May 28, 2026

Uh oh!

ikerexxe May 28, 2026

Uh oh!

ikerexxe May 28, 2026

Uh oh!

ikerexxe May 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

spoore1 commented May 27, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

ikerexxe May 28, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

ikerexxe left a comment

Choose a reason for hiding this comment

Uh oh!

ikerexxe May 28, 2026

Choose a reason for hiding this comment

Uh oh!

ikerexxe May 28, 2026

Choose a reason for hiding this comment

Uh oh!

ikerexxe May 28, 2026

Choose a reason for hiding this comment

Uh oh!

ikerexxe May 28, 2026

Choose a reason for hiding this comment

Uh oh!

ikerexxe May 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants