Skip to content

Conversation

@richm
Copy link
Contributor

@richm richm commented Dec 4, 2025

The old implemenation of previous: replaced would remove all files and reload the
firewall every time, even if not necessary - it wasn't technically idempotent, although
it would check the new files to see if they matched the old files, and report
changed: false if nothing actually changed state.

The new implementation uses an in-memory backend to apply the changes, then checks if
anything changed, and then removes the files and reloads firewall only if something
actually changed.

Signed-off-by: Rich Megginson rmeggins@redhat.com

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.

Sorry @richm, your pull request is larger than the review limit of 150000 diff characters

@richm richm force-pushed the previous-replaced-idempotent branch from 1248c50 to 1c35ffb Compare December 4, 2025 02:20
@codecov
Copy link

codecov bot commented Dec 4, 2025

Codecov Report

❌ Patch coverage is 23.02577% with 926 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.55%. Comparing base (2d7c4ba) to head (6ff5467).
⚠️ Report is 116 commits behind head on main.

Files with missing lines Patch % Lines
library/firewall_lib.py 21.78% 499 Missing ⚠️
module_utils/firewall_lsr/get_config.py 24.51% 425 Missing ⚠️
library/firewall_lib_facts.py 0.00% 2 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (2d7c4ba) and HEAD (6ff5467). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (2d7c4ba) HEAD (6ff5467)
sanity 1 0
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #310       +/-   ##
===========================================
- Coverage   61.09%   42.55%   -18.55%     
===========================================
  Files           2        3        +1     
  Lines         910     2277     +1367     
===========================================
+ Hits          556      969      +413     
- Misses        354     1308      +954     
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.

@richm richm force-pushed the previous-replaced-idempotent branch from 1c35ffb to 1f2d624 Compare December 4, 2025 03:16
if HAS_FIREWALLD:
firewall.config.FIREWALLD_POLICIES

HAS_POLICIES = True

Check notice

Code scanning / CodeQL

Unused global variable Note

The global variable 'HAS_POLICIES' is not used.

HAS_POLICIES = True
except AttributeError:
HAS_POLICIES = False

Check notice

Code scanning / CodeQL

Unused global variable Note

The global variable 'HAS_POLICIES' is not used.
try:
from firewall.functions import check_mac

HAS_CHECK_MAC = True

Check notice

Code scanning / CodeQL

Unused global variable Note

The global variable 'HAS_CHECK_MAC' is not used.

HAS_CHECK_MAC = True
except ImportError:
HAS_CHECK_MAC = False

Check notice

Code scanning / CodeQL

Unused global variable Note

The global variable 'HAS_CHECK_MAC' is not used.
@richm richm force-pushed the previous-replaced-idempotent branch 3 times, most recently from 7d5e884 to 81292e4 Compare December 4, 2025 03:43
@richm
Copy link
Contributor Author

richm commented Dec 4, 2025

[citest]

@richm richm force-pushed the previous-replaced-idempotent branch from 81292e4 to bb240fa Compare December 4, 2025 14:05
@richm
Copy link
Contributor Author

richm commented Dec 4, 2025

[citest]

@richm richm force-pushed the previous-replaced-idempotent branch from bb240fa to a697edc Compare December 4, 2025 16:44
@richm richm force-pushed the previous-replaced-idempotent branch 7 times, most recently from 8db7dc8 to d4a3ad6 Compare December 12, 2025 16:32
…nd reload unless necessary

The old implemenation of `previous: replaced` would remove all files and reload the
firewall every time, even if not necessary - it wasn't technically idempotent, although
it would check the new files to see if they matched the old files, and report
`changed: false` if nothing actually changed state.

The new implementation uses an in-memory backend to apply the changes, then checks if
anything changed, and then removes the files and reloads firewall only if something
actually changed.

Signed-off-by: Rich Megginson <rmeggins@redhat.com>
@richm richm force-pushed the previous-replaced-idempotent branch from d4a3ad6 to 6ff5467 Compare December 16, 2025 19:05
@richm
Copy link
Contributor Author

richm commented Dec 16, 2025

[citest]

@richm
Copy link
Contributor Author

richm commented Dec 16, 2025

[citest_bad]

@richm richm requested a review from spetrosi December 16, 2025 21:53
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.

1 participant