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

Test: sssclt test case #7457

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DDDas7
Copy link
Contributor

@DDDas7 DDDas7 commented Jun 24, 2024

Check for error if memcache_size_sid is set in nss section.

Check for error if memcache_size_sid is set in nss section.

Signed-off-by: Deepak Das <ddas@redhat.com>
@DDDas7 DDDas7 force-pushed the jira-rhel-28666-sssctl-config-check branch from 9ce763a to 1d02cac Compare June 24, 2024 15:23
@alexey-tikhonov
Copy link
Member

@DDDas7, thank you.

@danlavu , could you please check how/if this fits into testing plan?

Imo, checking for a single (stray) config option in a separate test isn't best approach. And this is hardly "sssctl" test. Rather "config" class.

@danlavu
Copy link

danlavu commented Jun 25, 2024

@alexey-tikhonov, I'm still determining. I told @madhuriupadhye, we can approve the test but tag it as deferred. I'm going to perform the housekeeping with a higher priority so we can get new cases in.

@alexey-tikhonov
Copy link
Member

@danlavu, any decision?

@danlavu danlavu removed the Deferred label Aug 21, 2024
@danlavu
Copy link

danlavu commented Aug 21, 2024

All of the sssctl tests need to be reworked, but it's not a priority. Let's go ahead and add the test and it'll get reworked with everything else.

Copy link

@danlavu danlavu left a comment

Choose a reason for hiding this comment

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

Other than the line break the test looks good, thank you.

client.sssd.nss["memcache_size_sid"] = "10"
client.sssd.start(check_config=False)
result = client.sssctl.config_check()
assert result.rc == 0, "Config-check failed"
Copy link

Choose a reason for hiding this comment

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

For readability, add a line break before the assertions.

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

@danlavu
Copy link

danlavu commented Oct 3, 2024

@DDDas7 rebase, add a blank line, look at covscan errors, this PR is close to being ready to be merged. Let's not forget about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants