fix: update source_mappings hash on project sync to prevent stale validation#1543
fix: update source_mappings hash on project sync to prevent stale validation#1543amasolov wants to merge 1 commit into
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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 (5)
📝 WalkthroughWalkthroughAdds an explicit activation-update workflow after syncing a rulebook: bulk-refreshes activations with empty ChangesRulebook Activation Sync Logic
Task logging and docs
Tests & Fixtures
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Comment |
amasolov
force-pushed
the
fix/update-source-mappings-hash-on-project-sync
branch
from
1afb23e to
bb6c136
Compare
![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
🧹 Nitpick comments (1)
src/aap_eda/services/project/imports.py (1)
225-251: ⚡ Quick winLimit this loop to activations that actually need per-record work.
This queryset now walks every activation for the rulebook, including
restart_on_project_update=Truerows with nosource_mappings, which are a no-op here because_auto_restart_activations()handles them later. Filtering those out would reduce ORM churn on large syncs. As per coding guidelines, Focus on major issues impacting performance, readability, maintainability and security.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aap_eda/services/project/imports.py` around lines 225 - 251, The loop is iterating all models.Activation rows for a rulebook including entries with restart_on_project_update=True and no source_mappings which are no-ops here; restrict the queryset before the loop to only activations that need per-record work by filtering for either restart_on_project_update=False OR source_mappings__isnull=False (or equivalent emptiness check) so the loop only processes activations requiring updates (those handled by this codepath that call _update_source_mappings_hash or update rulebook_rulesets/git_hash), leaving rows that need auto-restart to _auto_restart_activations().
🤖 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/services/project/imports.py`:
- Around line 241-248: The current code silently ignores invalid or unparseable
activation.source_mappings because _update_source_mappings_hash can return None;
change the flow so invalid mappings are surfaced: when
_update_source_mappings_hash returns None (or raises a parse/shape error) mark
the activation as failed/invalid (e.g., set an activation.failure_reason or
activation.invalid_source_mappings flag) and ensure this activation is removed
from restart/sync paths (do not append "source_mappings" to update_fields and
explicitly record the error on the activation so callers know to skip it). Apply
the same change for the analogous block handled by _update_source_mappings_hash
in lines 253-273 so all parse failures are recorded and such activations are
excluded from restart logic.
---
Nitpick comments:
In `@src/aap_eda/services/project/imports.py`:
- Around line 225-251: The loop is iterating all models.Activation rows for a
rulebook including entries with restart_on_project_update=True and no
source_mappings which are no-ops here; restrict the queryset before the loop to
only activations that need per-record work by filtering for either
restart_on_project_update=False OR source_mappings__isnull=False (or equivalent
emptiness check) so the loop only processes activations requiring updates (those
handled by this codepath that call _update_source_mappings_hash or update
rulebook_rulesets/git_hash), leaving rows that need auto-restart to
_auto_restart_activations().
🪄 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: e2df5ff1-ae5a-418f-811c-aed0eb524cca
📒 Files selected for processing (5)
src/aap_eda/services/project/imports.pysrc/aap_eda/tasks/project.pytests/integration/services/data/project-07/extensions/eda/rulebooks/hello_events.ymltests/integration/services/test_project_import.pytests/integration/tasks/test_projects.py
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1543 +/- ##
=======================================
Coverage 91.99% 92.00%
=======================================
Files 241 241
Lines 10958 10984 +26
=======================================
+ Hits 10081 10106 +25
- Misses 877 878 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@amasolov Thanks for the contribution! The skip guard this PR removes was added intentionally in AAP-65811 (PR #1480). The reason auto-restart is skipped for activations with Updating The designed solution (AAP-65811) is: skip auto-restart for source-mapped activations, leave them running with old-but-working swapped content, and surface a computed |
|
@AlexSCorey, could you please check if the warning message is visible in the UI? |
|
@hsong-rh Thank you for the detailed review. You're absolutely right that the skip guard was intentional, and I missed the I've revised the approach in d7427ea:
This fixes the original bug (stale |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/services/test_project_import.py (1)
450-454: 💤 Low valueAdd a
statusguard to the happy-path assertionsThe test verifies that
source_mappingsis updated but never assertsactivation.status != ActivationStatus.ERROR. If the sync implementation accidentally writes ERROR and still updates the YAML (e.g., partial execution before the guard), the test passes incorrectly.🛡️ Suggested addition
activation.refresh_from_db() updated_mappings = yaml.safe_load(activation.source_mappings) +assert activation.status != ActivationStatus.ERROR assert updated_mappings[0]["rulebook_hash"] == new_hash assert activation.rulebook_rulesets == swapped_rulesets assert activation.rulebook_rulesets_sha256 == old_hash🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/services/test_project_import.py` around lines 450 - 454, After refreshing the activation (activation.refresh_from_db()), add an assertion that the activation's status is not ERROR to prevent false-positive happy-paths; specifically assert activation.status != ActivationStatus.ERROR (or assert activation.status == ActivationStatus.ACTIVE if that aligns with expected state) and ensure ActivationStatus is imported/available in the test so the new guard references the ActivationStatus enum rather than a raw string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/integration/services/test_project_import.py`:
- Around line 450-454: After refreshing the activation
(activation.refresh_from_db()), add an assertion that the activation's status is
not ERROR to prevent false-positive happy-paths; specifically assert
activation.status != ActivationStatus.ERROR (or assert activation.status ==
ActivationStatus.ACTIVE if that aligns with expected state) and ensure
ActivationStatus is imported/available in the test so the new guard references
the ActivationStatus enum rather than a raw string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 4b9377fc-a412-434f-b6dd-f0e73e3fad57
📒 Files selected for processing (3)
src/aap_eda/services/project/imports.pysrc/aap_eda/tasks/project.pytests/integration/services/test_project_import.py
✅ Files skipped from review due to trivial changes (1)
- src/aap_eda/tasks/project.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/aap_eda/services/project/imports.py
|
Missing from description:
|
|
/run-e2e |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/services/test_project_import.py (1)
458-503: ⚡ Quick winCover the non-dict
source_mappingsregression path.This adds coverage for syntactically invalid YAML, but the new guard in
src/aap_eda/services/project/imports.pyat Line 288 is hit by valid YAML with the wrong shape, e.g.- 1. Adding that case here would lock in the recentTypeErrorfix and close the remaining uncovered branch.Proposed test tweak
-@pytest.mark.django_db -def test_sync_marks_error_on_unparseable_source_mappings( +@pytest.mark.django_db +@pytest.mark.parametrize( + "source_mappings", + [ + "{{{ not valid yaml", + yaml.dump([1], default_flow_style=False), + ], +) +def test_sync_marks_error_on_invalid_source_mappings( + source_mappings: str, default_organization: models.Organization, storage_save_patch, service_tempdir_patch, ): - """An activation whose source_mappings cannot be parsed as YAML - is marked with ActivationStatus.ERROR after a content-changing sync.""" + """Invalid source_mappings are marked with ActivationStatus.ERROR + after a content-changing sync.""" git_mock_v1 = _mock_git_clone( "project-02", "aaa111aaa111aaa111aaa111aaa111aaa111aaa1" ) @@ activation = models.Activation.objects.create( name="bad-mappings-activation", project=project, organization=default_organization, rulebook=rulebook, rulebook_name=rulebook.name, rulebook_rulesets=rulebook.rulesets, rulebook_rulesets_sha256=get_rulebook_hash(rulebook.rulesets), - source_mappings="{{{ not valid yaml", + source_mappings=source_mappings, is_enabled=True, restart_on_project_update=False, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/services/test_project_import.py` around lines 458 - 503, Update the test_sync_marks_error_on_unparseable_source_mappings test to also cover the non-dict source_mappings regression: create a second Activation (or modify the existing one) whose source_mappings is syntactically valid YAML but not a mapping (e.g. "- 1" or "[]"), call ProjectImportService.sync_project (or service_v2.sync_project) to trigger the same import flow, then refresh the activation and assert activation.status == ActivationStatus.ERROR and that the status_message contains "source_mappings could not be updated"; this ensures the guard added in src/aap_eda/services/project/imports.py (around the check handling non-dict parsed YAML) and the Activation handling are tested.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/integration/services/test_project_import.py`:
- Around line 458-503: Update the
test_sync_marks_error_on_unparseable_source_mappings test to also cover the
non-dict source_mappings regression: create a second Activation (or modify the
existing one) whose source_mappings is syntactically valid YAML but not a
mapping (e.g. "- 1" or "[]"), call ProjectImportService.sync_project (or
service_v2.sync_project) to trigger the same import flow, then refresh the
activation and assert activation.status == ActivationStatus.ERROR and that the
status_message contains "source_mappings could not be updated"; this ensures the
guard added in src/aap_eda/services/project/imports.py (around the check
handling non-dict parsed YAML) and the Activation handling are tested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 165799e6-6c60-4e5f-ac38-6f0271d15207
📒 Files selected for processing (2)
src/aap_eda/services/project/imports.pytests/integration/services/test_project_import.py
amasolov
force-pushed
the
fix/update-source-mappings-hash-on-project-sync
branch
from
d695eaa to
a9c4579
Compare
…idation When a project sync changes rulebook content, the rulebook_hash embedded in each activation's source_mappings was not updated. This caused restart and enable operations to fail with: "Rulebook has changed since the sources were mapped. Please reattach event streams" Add _update_activations_for_rulebook() which: - Bulk-updates non-mapped, non-auto-restart activations as before - Iterates source-mapped activations to rewrite the embedded rulebook_hash inside source_mappings YAML - Marks activations with unparseable source_mappings as ERROR - Preserves rulebook_rulesets for source-mapped activations (the post-swap content from swap_event_stream_sources()) Also updates the _auto_restart_activations docstring to clarify the swap_event_stream_sources() contract and consolidates multi-line logger f-strings. Closes ansible#1542 Signed-off-by: Alexey Masolov <amasolov@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
amasolov
force-pushed
the
fix/update-source-mappings-hash-on-project-sync
branch
from
a9c4579 to
a5ac27a
Compare
|
|
/run-e2e |
6 participants
Summary
_sync_rulebook()to rewrite therulebook_hashinside each affected activation'ssource_mappingswhen rulebook content changes during a project sync_check_and_restart_activation()that skipped auto-restart for activations with source_mappings, since the hash is now kept in syncTest plan
ruff checkandruff format --checkpasstest_sync_updates_source_mappings_hashverifies hash is updated after synctest_auto_restart_restarts_activation_with_source_mappingsverifies auto-restart now proceedstest_auto_restart_mixed_source_mappings_activationsverifies both mapped and unmapped activations are restartedTracking
Closes #1542
Made with Cursor
Summary by CodeRabbit
New Features
Tests