[AAP-72873] fix: preserve event stream source swap after project sync#1540
[AAP-72873] fix: preserve event stream source swap after project sync#1540B-Whitt wants to merge 1 commit into
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe changes modify Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 16 minutes and 1 second.Comment |
|
/run-e2e |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/aap_eda/tasks/project.py`:
- Line 368: activation.source_mappings is being checked by raw string truthiness
which misdetects values like "[]" or whitespace as present; instead, parse or
normalize the YAML string and check for an actual empty mapping. Update the
logic around the activation.source_mappings check (the branch that currently
reads `if not activation.source_mappings`) to first normalize the string (e.g.,
strip() and compare against ""/"[]"/"{}") or better, safe_load the YAML and test
whether the resulting object is None/empty list/empty dict; treat those as empty
so the ruleset refresh path runs when mappings were cleared.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 8ad45c6c-e492-416f-8dfc-7796d4ad3f5d
📒 Files selected for processing (2)
src/aap_eda/tasks/project.pytests/integration/tasks/test_projects.py
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1540 +/- ##
==========================================
- Coverage 91.98% 91.97% -0.01%
==========================================
Files 241 241
Lines 10955 10968 +13
==========================================
+ Hits 10077 10088 +11
- Misses 878 880 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
There's a second write site that also overwrites In models.Activation.objects.filter(
rulebook=rulebook,
restart_on_project_update=False,
).update(
rulebook_rulesets=rulebook.rulesets,
rulebook_rulesets_sha256=new_sha256,
git_hash=git_hash,
)This runs for activations with no_restart = models.Activation.objects.filter(
rulebook=rulebook,
restart_on_project_update=False,
)
no_restart.filter(source_mappings="").update(
rulebook_rulesets=rulebook.rulesets,
rulebook_rulesets_sha256=new_sha256,
git_hash=git_hash,
)
no_restart.exclude(source_mappings="").update(
git_hash=git_hash,
)This should be folded into this PR, without it the bug can still reproduce via the |
B-Whitt
force-pushed
the
fix/preserve-event-stream-source-swap-AAP-72873
branch
from
c2b7881 to
879b5d3
Compare
…AP-72873) _update_activation_content() unconditionally overwrote rulebook_rulesets with the raw (unswapped) rulebook from the DB, losing the event stream source swap (webhook → pg_listener) for activations with source_mappings. This caused ansible-rulebook to bind to the original webhook port, failing on podman and using the wrong source on K8s. Guard the rulesets overwrite with a source_mappings check — activations that have event stream mappings keep their swapped rulesets, consistent with how _auto_restart_activations() already skips source-mapped activations. Assisted by: Claude Opus
B-Whitt
force-pushed
the
fix/preserve-event-stream-source-swap-AAP-72873
branch
from
879b5d3 to
0222777
Compare
|
/run-e2e |
|
hsong-rh
left a comment
hsong-rh
left a comment
There was a problem hiding this comment.
Hold the fix, this behavior is by design. See details in https://issues.redhat.com/browse/AAP-65811
|
Related PR: #1543 |
|
closed in favor of #1547 |
|
closed in favor of #1547 |
3 participants
What is being changed?
_update_activation_content()insrc/aap_eda/tasks/project.pynow skips therulebook_rulesetsoverwrite for activations that havesource_mappings. Previously it unconditionally overwrote the activation's cached rulesets with the raw rulebook from the DB, which does not have the event stream source swap applied.Why is this change needed?
When a project sync completes for activations with
update_revision_on_launch=True,_resume_waiting_activations()calls_update_activation_content()which overwroterulebook_rulesetswith the original (unswapped) rulebook from the DB. This lost the event stream source swap (webhook → pg_listener), causingansible-rulebookto bind to the original webhook port. On podman this fails with "port already in use"; on K8s the activation runs but with the wrong source type.Jira: https://redhat.atlassian.net/browse/AAP-72873
How does this change address the issue?
A single guard:
if not activation.source_mappings:wraps the rulesets overwrite in_update_activation_content(). Activations with event stream source mappings keep their already-swapped rulesets intact. Onlygit_hashis updated unconditionally.This is consistent with how
_auto_restart_activations()already skips source-mapped activations when content changes (line 254).Does this change introduce any new dependencies, blockers or breaking changes?
No new imports or dependencies. The change is a single guard condition.
How it can be tested?
Automated: 3 regression tests in
tests/integration/tasks/test_projects.py:test_update_activation_content_preserves_source_mapped_rulesets— verifies swapped rulesets are not overwrittentest_update_activation_content_updates_rulesets_without_source_mappings— verifies rulesets refresh still works for normal activationstest_resume_waiting_preserves_event_stream_swap— end-to-end test of the sync→resume pathAll 68 tests in
test_projects.pypass locally (0 failures).Manual reproduction (from Jira):
ansible.eda.webhookwith a port configuredupdate_revision_on_launch=Trueon the projectTested on AAP 2.7 (aap-dev) — activation runs successfully through the sync→resume path with the swapped rulesets preserved.
Assisted by: Claude Opus
Summary by CodeRabbit
Bug Fixes
Tests