[AAP-76816] Add progress reporting during merge task execution#118
[AAP-76816] Add progress reporting during merge task execution#118john-westcott-iv wants to merge 5 commits into
Conversation
…ng pagination Replace RuntimeError with a warning log when role assignment count changes between pages, and move the generator iteration inside the try/except block so errors during pagination are caught instead of crashing the migration. Add a separate try/except around give_permission so permission errors get their own accurate error message. Track services with count drift and display a summary warning at the end of migration advising a re-run. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…dency repos When the PR checkout script looks for a matching branch in always-clone repos (e.g., django-ansible-base), fall back to devel instead of failing if the branch doesn't exist. Feature branches are gateway-specific and don't always have a corresponding branch in every dependency repo. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address CodeRabbit review: assert logger.warning is called with correct message when count changes, and verify the warning fires only once when the count stabilizes on subsequent pages. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…in dependency repos" This reverts commit 4bda29d.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Dostonbek1
left a comment
There was a problem hiding this comment.
Code Review Summary
This PR adds operator-friendly progress reporting to the long-running migrate_service_data command, enabling monitoring via podman logs. The approach is well-designed: threshold-based logging at 5% intervals avoids flooding logs while giving visibility into progress. The service index addition (Processing service: controller (1/3)) is a nice touch.
Files Reviewed: 2 files
Comments Posted: 2 review comments
Issues Found
- 0 security/safety issues
- 1 minor correctness observation (progress accuracy during count drift)
- 1 minor suggestion (extra API call)
Overall Assessment
LGTM with minor observations. The _log_progress method is clean and well-tested, with good handling of edge cases (zero items, bookends, no-duplicate-100%). The threshold logic is correct.
Highlights
- Clean separation of progress logic into
_log_progresswith clear docstring - Good test coverage: threshold crossings, zero items, small counts, bookends, and no-duplicate-100%
- Using
logger.info(notself.stdout.write) is the right choice for container log visibility PROGRESS_STEPas a class constant makes it easy to tune
Note on PR scope
This PR includes commits from #115 (AAP-76813). My review focuses on the AAP-76816 progress-reporting changes specifically. I reviewed #115 separately.
| json_response = method(filters=filters).json() | ||
| if total_count is None: | ||
| total_count = json_response.get('count', 0) | ||
| self._current_assignment_total = total_count |
There was a problem hiding this comment.
Observation (non-blocking): _current_assignment_total is set here on the first page but is NOT updated when count drift occurs (the elif branch below updates local total_count but not self._current_assignment_total). This means:
- If items are added during migration, progress percentages can exceed 100% (e.g.,
"87000/86000 (100%)"followed by more processing with no further progress output) - If items are removed, the 100% bookend may never fire since
processedwon't reach the originaltotal
For a progress indicator this is acceptable behavior (and fixing it would require updating _current_assignment_total in the drift branch, which couples the generator further to the progress state). Just noting for awareness — if operators report confusing progress output, this is likely why.
| } | ||
|
|
||
| # Get initial total count for progress reporting | ||
| initial_count_response = self.client.list_resources(filters=api_call_filters).json() |
There was a problem hiding this comment.
Minor note: This issues an extra list_resources API call solely for the count. The very first iteration of the while True loop below will make the same call again (via _get_filtered_resources). For a long-running migration doing thousands of calls, one extra round-trip per resource type is negligible — but if you ever refactor _get_filtered_resources to return the full response (or just the count alongside results), you could eliminate this.
Add percentage-based progress logging to migrate_service_data so operators can monitor long-running migrations. Progress is reported at every 5% threshold for both resource migration and role assignment migration, with bookend messages at start and 100% completion. Uses logger.info so output appears in container logs. Service processing loop now shows index. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fb9ff1f to
9ff72cf
Compare
|
DVCS PR Check Results: PR appears valid (JIRA key(s) found) |
Summary
migrate_service_dataat 5% threshold intervalslogger.infoso output appears inpodman logscontainer outputProcessing service: controller (1/3)Format
Test plan
_log_progressthreshold logic, edge cases (0 items, small counts, no duplicate 100%)podman logsoutputDepends on: #115 (AAP-76813)
🤖 Generated with Claude Code