Skip to content

Conversation

@martinpitt
Copy link
Contributor

@martinpitt martinpitt commented May 13, 2025

These are two prerequisites for eventual bootc build support. They are tested in my bootc-container-test branch, and so far these parts work fine. These two commits are fairly intrusive but conceptually simple, so it may make sense to already land them. I also want to make sure that it doesn't break any of the qemu/TF tests.

Summary by Sourcery

Prepare for bootc build support by migrating tests to offline mode and enhancing service handling.

Enhancements:

  • Switch all ansible test scripts to use firewall-offline-cmd instead of firewall-cmd for offline configuration
  • Replace service_facts with systemctl is-enabled checks and split disable/stop of conflicting services into separate tasks with conditional execution
  • Wrap non-idempotent --set-default-zone calls in idempotent shell logic within tests

@sourcery-ai
Copy link

sourcery-ai bot commented May 13, 2025

Reviewer's Guide

This PR prepares for bootc support by converting all permanent firewall operations in the test suites to use firewall-offline-cmd, refactoring conflicting-service disable tasks for container compatibility, and adding guarded reload steps that only invoke firewall-cmd --reload on booted systems.

File-Level Changes

Change Details Files
Switch integration tests to offline configuration mode
  • Replace firewall-cmd --permanent calls with firewall-offline-cmd
  • Convert all zone, service, and ipset query/reset operations to firewall-offline-cmd
  • Adjust shell wrappers to handle non-idempotent set-default-zone invocations
tests/tests_ansible.yml
tests/tests_zone.yml
tests/tests_ipsets.yml
tests/tests_service.yml
tests/tests_firewall_fact.yml
tests/tests_target.yml
Refactor conflicting-service disable tasks for container environments
  • Use systemctl is-enabled in a loop to gather service status
  • Separate disable and stop actions based on each service’s enabled state
  • Remove service_facts module dependency in favor of direct checks
tasks/main.yml
Guard firewall reloads to run only on booted systems
  • Introduce dedicated reload tasks that call firewall-cmd --reload behind conditions
  • Ensure reload steps are only executed when the system is confirmed booted
tests/tests_firewall_fact.yml
tests/tests_zone.yml

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

@codecov
Copy link

codecov bot commented May 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.56%. Comparing base (2d7c4ba) to head (a215166).
Report is 66 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #270      +/-   ##
==========================================
- Coverage   61.09%   60.56%   -0.54%     
==========================================
  Files           2        2              
  Lines         910      923      +13     
==========================================
+ Hits          556      559       +3     
- Misses        354      364      +10     
Flag Coverage Δ
sanity ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@martinpitt martinpitt changed the title ci: Move tests to firewall-offline-cmd; Make disabling of conflicting services work in container environments ci: Two prerequisites for bootc support May 13, 2025
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

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.

@martinpitt martinpitt marked this pull request as draft May 13, 2025 10:08
We are only interested in the permanent configuration anyway, so we
can drop the "firewalld is running" assumption and use
`firewall-offline-cmd` instead to query the on-disk state directly. This
also works in container builds.
@martinpitt
Copy link
Contributor Author

[citest]

@martinpitt martinpitt marked this pull request as ready for review May 13, 2025 10:50
@martinpitt martinpitt requested a review from richm May 13, 2025 10:50
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

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

  • Instead of using custom shell conditionals for --set-default-zone, leverage Ansible’s built-in idempotence (e.g. changed_when or a dedicated module parameter) to avoid brittle shell logic.
  • The tests still directly invoke firewall-cmd --reload; wrap those reload steps in the same boot-check guard you added elsewhere or centralize reload into a single task to ensure consistency.
  • In tasks/main.yml you manually parse systemctl is-enabled—you can simplify by using a single systemd module call with enabled: false and state: stopped to handle disabling and stopping services in one go.
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.

@richm
Copy link
Contributor

richm commented May 13, 2025

I'm not sure how this works to simply swap firewall-cmd with firewall-offline-cmd, unless all of our tests always write the changes to the backing XML files.

@martinpitt
Copy link
Contributor Author

I'm not sure how this works to simply swap firewall-cmd with firewall-offline-cmd, unless all of our tests always write the changes to the backing XML files.

The only places that use runtime: true are tests_ansible.yml and tests_ipsets.yml. But at these places they don't actually check the results. If they would, or we ever get a test that only covers runtime: true, we'd of course keep firewall-cmd and guard it with when: __firewall_is_booted.

All other places have permanent: true and called firewall-cmd with --permanent anyway, i.e. it's functionally equivalent.

…ments

Cause: The `firewall_disable_conflicting_services` option did not work
in container build environments, as Ansible's `service_facts` completely
fail there.

Consequence: Running the role with that option during e.g. a bootc image
build errored out with "Failed to find any services".

Fix: The role only actually needs to know about the status of a small
list of known conflicting services. Query these with `systemctl is-enabled`
which works fine in container builds.

Drop the obsolete `service_mgr == 'systemd'` check. All our supported platforms
run systemd, and we don't test anything else.
@richm
Copy link
Contributor

richm commented May 14, 2025

[citest]

@richm richm merged commit e88b15e into linux-system-roles:main May 14, 2025
34 of 35 checks passed
@martinpitt martinpitt deleted the offline-prep branch May 14, 2025 18:37
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