Skip to content

DONOTMERGE: Temporary commit for testing - #8980

Draft
spoore1 wants to merge 1 commit into
SSSD:masterfrom
spoore1:adcs_smartcard_setup_v2
Draft

DONOTMERGE: Temporary commit for testing#8980
spoore1 wants to merge 1 commit into
SSSD:masterfrom
spoore1:adcs_smartcard_setup_v2

Conversation

@spoore1

@spoore1 spoore1 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@spoore1
spoore1 marked this pull request as draft July 21, 2026 12:44

@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 introduces a new system test for Active Directory smartcard authentication (test_smartcard_ad.py) and updates the test framework requirements. Feedback on the changes includes reverting the dependency on a personal fork in requirements.txt back to the official upstream repository, removing unused imports and commented-out code blocks in the new test file, and optimizing the user lookup polling loop to avoid an unnecessary initial sleep delay.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +8 to +9
#git+https://github.com/SSSD/sssd-test-framework
git+https://github.com/spoore1/sssd-test-framework@adcs_smartcard_setup_v2

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The requirements file has been modified to point to a personal fork and branch (spoore1/sssd-test-framework@adcs_smartcard_setup_v2) instead of the official upstream repository. This must be reverted to the official SSSD repository before merging to ensure stability and avoid supply chain risks.

git+https://github.com/SSSD/sssd-test-framework

Comment on lines +14 to +15
from sssd_test_framework.roles.generic import GenericADProvider
from sssd_test_framework.topology import KnownTopology, KnownTopologyGroup

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The imports GenericADProvider and KnownTopologyGroup are not used anywhere in this test file. Removing unused imports keeps the codebase clean and adheres to PEP 8 guidelines.

Suggested change
from sssd_test_framework.roles.generic import GenericADProvider
from sssd_test_framework.topology import KnownTopology, KnownTopologyGroup
from sssd_test_framework.topology import KnownTopology

Comment on lines +92 to +99

# client.host.conn.run("rm -f /etc/krb5.conf /etc/krb5.keytab", raise_on_error=False)
# client.host.conn.run(
# f"realm join {ad.domain}",
# input=ad.host.adminpw,
# raise_on_error=False,
# )

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

This block of commented-out code should be removed to keep the test suite clean and maintainable.

Suggested change
# client.host.conn.run("rm -f /etc/krb5.conf /etc/krb5.keytab", raise_on_error=False)
# client.host.conn.run(
# f"realm join {ad.domain}",
# input=ad.host.adminpw,
# raise_on_error=False,
# )

Comment on lines +105 to +112
#
# client.host.conn.run(
# r"sed -i 's/\[default=2 ignore=ignore success=ok\]\s*pam_localuser.so/"
# r"[default=ignore ignore=ignore success=ok] pam_localuser.so/' "
# r"/etc/pam.d/system-auth",
# )
#
# client.sssd.config_load()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

This block of commented-out code should be removed to keep the test suite clean and maintainable.

Suggested change
#
# client.host.conn.run(
# r"sed -i 's/\[default=2 ignore=ignore success=ok\]\s*pam_localuser.so/"
# r"[default=ignore ignore=ignore success=ok] pam_localuser.so/' "
# r"/etc/pam.d/system-auth",
# )
#
# client.sssd.config_load()

Comment on lines +140 to +146
for _attempt in range(10):
time.sleep(3)
result = client.tools.getent.passwd(fqdn_user)
if result is not None:
break
if _attempt == 2:
client.host.conn.run("sss_cache -E", raise_on_error=False)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The loop always sleeps for 3 seconds on the very first iteration before checking if the user is available. Checking the passwd database first before sleeping avoids an unnecessary 3-second delay in the happy path, improving test suite efficiency.

Suggested change
for _attempt in range(10):
time.sleep(3)
result = client.tools.getent.passwd(fqdn_user)
if result is not None:
break
if _attempt == 2:
client.host.conn.run("sss_cache -E", raise_on_error=False)
for _attempt in range(10):
result = client.tools.getent.passwd(fqdn_user)
if result is not None:
break
if _attempt == 2:
client.host.conn.run("sss_cache -E", raise_on_error=False)
time.sleep(3)

import pytest
from sssd_test_framework.roles.ad import AD
from sssd_test_framework.roles.client import Client
from sssd_test_framework.roles.generic import GenericADProvider
from sssd_test_framework.roles.ad import AD
from sssd_test_framework.roles.client import Client
from sssd_test_framework.roles.generic import GenericADProvider
from sssd_test_framework.topology import KnownTopology, KnownTopologyGroup
username = "adcertuser1"
try:
ad.user(username).delete()
except Exception:
Comment on lines +115 to +116
# if "config_file_version" in client.sssd.sssd:
# del client.sssd.sssd["config_file_version"]
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.

2 participants