tests: Add an actual integration test #101

martinpitt · 2025-06-06T12:21:07Z

tests_default.yml doesn't actually do anything other than making sure that the role succees. It does not have any assertions.

Add tests_all_settings.yml based on examples/playbook_with_vars.yml which validates that the role has the desired effect.

This uncovers a bug that the custom authselect policy is not applied properly.

Yak shaving - I was about to add a bootc end-to-end test, then noticed that this role is lacking a test, and wrote one.

Summary by Sourcery

Add a comprehensive integration test for the pam_pwd role that applies all configuration options and verifies the resulting pwquality, PAM, and faillock settings, and ensure the authselect package is installed before configuring policies.

Bug Fixes:

Ensure authselect package is installed prior to configuring custom policies

Documentation:

Clarify platform availability for pam_pwd_policy_name setting in README

Tests:

Introduce tests_all_settings.yml to run the role with all parameters and assert pwquality.conf, PAM profiles, and faillock.conf

sourcery-ai · 2025-06-06T12:21:12Z

Reviewer's Guide

Introduces a comprehensive integration test for the pam_pwd role covering all configuration paths, fixes a missing authselect installation step to enable custom policy application, and updates documentation to clarify pam_pwd_policy_name availability.

File-Level Changes

Change	Details	Files
Add full integration test for pam_pwd role	Add tests_all_settings.yml to run the role with every parameter Assert pwquality.conf contains expected settings Verify PAM profiles and faillock.conf reflect all parameters	`tests/tests_all_settings.yml`
Ensure authselect package is installed	Add a package task for authselect before listing and configuring profiles	`tasks/setup/default.yml`
Clarify pam_pwd_policy_name variable in documentation	Update README to note pam_pwd_policy_name is unavailable on RHEL 7	`README.md`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

tests/tests_all_settings.yml

martinpitt · 2025-06-06T12:28:47Z

@Tronde can you please have a quick look? The intention is surely that the changes actually become active? Is that working for you? If so, which OS are you using? Thanks!

martinpitt · 2025-06-06T12:33:50Z

@Tronde unping, sorry -- forgot flush_handlers 🙈

martinpitt · 2025-06-06T12:42:11Z

Much better -- the tests now only fail in c9-container, for a rather shallow reason:

TASK [linux-system-roles.pam_pwd : List authselect profiles] *******************
task path: /home/runner/work/pam_pwd/pam_pwd/tests/roles/linux-system-roles.pam_pwd/tasks/setup/default.yml:29
fatal: [sut]: FAILED! => {"changed": false, "cmd": "authselect list", "msg": "[Errno 2] No such file or directory: b'authselect'", "rc": 2, "stderr": "", "stderr_lines": [], "stdout": "", "stdout_lines": []}

Fixing..

"RHEL 8 only" sounds "only for old OS" from today's perspective. When it was introduced, that was probably "the future". There is only a special case for RHEL/CentOS 7 where `authselect` does not yet exist, so the setting does not apply there.

Cause: Not every environment has `authselect` installed, in particular the standard CentOS 9 container. Consequence: The role failed trying to call `authselect` in these environments. Fix: Ensure that the authselect package is installed.

sourcery-ai

Hey @martinpitt - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟢 Review instructions: all looks good
🟢 Testing: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

spetrosi · 2025-06-06T15:23:59Z

tasks/setup/default.yml

    state: present
    backup: true

+- name: Ensure authselect is installed


Should this be done on EL 7 since above you mention that pam_pwd_policy_name is not supported on EL7?

Ah, maybe not -- but there are no tests running for RHEL 7 anyway, so this is all untested code 😢 I'll add an exception though, I want to re-push anyway as I have a bootc integration test as well.

Correction, this is actually fine. RHEL 7 uses tasks/setup/RedHat_7.yml instead, which is completely different. default.yml unconditionally calls authselect, so it should also unconditionally install it.

And correction #2: our TF tests do run on CentOS 7, I just didn't trigger them in the previous round. And they indeed fail, but not on this package installation, but the assertions -- no authselect. I need to conditionalize the test.

martinpitt · 2025-06-06T16:19:47Z

later RHEL 7 failure, once more with feeling. Sorry @spetrosi for the premature review request, I'm not yet used to the automatic requests after sending a PR (will keep at draft while tests are still red)

tests_default.yml doesn't actually do anything other than making sure that the role succees. It does not have any assertions. Add tests_all_settings.yml based on examples/playbook_with_vars.yml which validates that the role has the desired effect.

See https://issues.redhat.com/browse/RHEL-78157

martinpitt · 2025-06-06T16:31:19Z

[citest]

sourcery-ai

Hey @martinpitt - I've reviewed your changes - here's some feedback:

Wrap the new authselect package installation in an OS/version conditional (e.g. RHEL 8+) so it doesn’t fail on systems that lack that package.
Add an assert that the custom authselect profile (pam_pwd_policy_name) is actually activated to ensure the role truly applies it.
Consider using Ansible’s ini lookup or ini_file module in the tests instead of grepping with cat for more robust config validation.

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟢 Review instructions: all looks good
🟢 Testing: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

martinpitt commented Jun 6, 2025

View reviewed changes

tests/tests_all_settings.yml Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

martinpitt force-pushed the bootc-e2e branch from 13bf2dd to df82401 Compare June 6, 2025 12:27

martinpitt force-pushed the bootc-e2e branch from df82401 to b06ab5e Compare June 6, 2025 12:32

martinpitt added 2 commits June 6, 2025 14:48

fix: Install authselect package if required

0d0f492

Cause: Not every environment has `authselect` installed, in particular the standard CentOS 9 container. Consequence: The role failed trying to call `authselect` in these environments. Fix: Ensure that the authselect package is installed.

martinpitt force-pushed the bootc-e2e branch from b06ab5e to c593e25 Compare June 6, 2025 12:49

martinpitt marked this pull request as ready for review June 6, 2025 14:05

martinpitt requested review from richm and spetrosi as code owners June 6, 2025 14:05

sourcery-ai bot reviewed Jun 6, 2025

View reviewed changes

spetrosi reviewed Jun 6, 2025

View reviewed changes

spetrosi previously approved these changes Jun 6, 2025

View reviewed changes

martinpitt dismissed spetrosi’s stale review via e7574e7 June 6, 2025 16:06

martinpitt force-pushed the bootc-e2e branch from c948895 to e7574e7 Compare June 6, 2025 16:06

martinpitt marked this pull request as draft June 6, 2025 16:15

martinpitt force-pushed the bootc-e2e branch from e7574e7 to 07813d5 Compare June 6, 2025 16:19

martinpitt added 2 commits June 6, 2025 18:30

tests: Add an actual integration test

672fd5b

tests_default.yml doesn't actually do anything other than making sure that the role succees. It does not have any assertions. Add tests_all_settings.yml based on examples/playbook_with_vars.yml which validates that the role has the desired effect.

tests: Update tests_all_settings to do bootc end-to-end validation

8869f73

See https://issues.redhat.com/browse/RHEL-78157

martinpitt force-pushed the bootc-e2e branch from 07813d5 to 8869f73 Compare June 6, 2025 16:30

martinpitt marked this pull request as ready for review June 6, 2025 17:39

martinpitt requested a review from spetrosi June 6, 2025 17:40

sourcery-ai bot reviewed Jun 6, 2025

View reviewed changes

richm approved these changes Jun 9, 2025

View reviewed changes

martinpitt merged commit ecda613 into linux-system-roles:main Jun 10, 2025
27 checks passed

martinpitt deleted the bootc-e2e branch June 10, 2025 05:27

tests: Add an actual integration test #101

tests: Add an actual integration test #101

Uh oh!

Conversation

martinpitt commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Sourcery

Uh oh!

sourcery-ai bot commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer's Guide

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

Uh oh!

Uh oh!

This comment was marked as resolved.

martinpitt commented Jun 6, 2025

Uh oh!

martinpitt commented Jun 6, 2025

Uh oh!

martinpitt commented Jun 6, 2025

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

spetrosi Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

martinpitt Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

martinpitt Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

martinpitt Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

martinpitt commented Jun 6, 2025

Uh oh!

martinpitt commented Jun 6, 2025

Uh oh!

sourcery-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

martinpitt commented Jun 6, 2025 •

edited

Loading

sourcery-ai bot commented Jun 6, 2025 •

edited

Loading