[AAP-72504] Add configurable page_size and jwt_expiration for resource sync#991
Open
huffmanca wants to merge 1 commit intoansible:develfrom
Open
[AAP-72504] Add configurable page_size and jwt_expiration for resource sync#991huffmanca wants to merge 1 commit intoansible:develfrom
huffmanca wants to merge 1 commit intoansible:develfrom
Conversation
…e sync Allow operators to tune RESOURCE_SYNC_PAGE_SIZE and RESOURCE_SYNC_JWT_EXPIRATION via Django settings. These are read automatically by SyncExecutor and create_api_client(), so consuming services (tower, hub, EDA) pick up new values without code changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
DVCS PR Check Results: PR appears valid (JIRA key(s) found) |
Contributor
📝 WalkthroughWalkthroughThe changes add configurable pagination and JWT expiration to the resource synchronization layer. New settings keys control page size (default 50) and JWT expiration (default 60), exposed via command-line parameter and threaded through the sync executor to the remote assignment fetcher. 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 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible_base/resource_registry/tasks/sync.py`:
- Around line 194-197: The constructor for the sync class currently accepts
page_size without validation (see __init__ and the page_size attribute) which
allows 0/negative/non-int values to silently enable partial-sync; update
__init__ to coerce and validate page_size: if page_size is not None attempt
int(page_size) and if conversion fails or the value is <= 0, fall back to a safe
default from getattr(settings, 'RESOURCE_SYNC_PAGE_SIZE', 50) (also validate
that setting as an int > 0 and fall back to 50 if not); assign the validated
positive int to self.page_size and ensure the same validation logic is applied
where page_size is read/used elsewhere (referenced around the other occurrence
noted) so pagination calls never receive non-positive or non-integer values.
- Line 112: Validate and sanitize settings.RESOURCE_SYNC_JWT_EXPIRATION before
assigning to params["jwt_expiration"]: ensure the value from settings is an
integer > 0 (coerce with int() safely), and if it's missing, non-numeric, or
non-positive, replace it with the default 60 (and optionally log/warn). Modify
the assignment around params["jwt_expiration"] in sync.py to perform this
check/coercion rather than forwarding the raw settings value.
🪄 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: b37132a0-ff60-4069-ab05-d2f971bf1cf3
📒 Files selected for processing (4)
ansible_base/lib/dynamic_config/settings_logic.pyansible_base/resource_registry/management/commands/resource_sync.pyansible_base/resource_registry/tasks/sync.pytest_app/tests/resource_registry/test_resource_sync.py
| if not service_path: | ||
| raise ValueError("RESOURCE_SERVICE_PATH is not set.") | ||
| params["service_path"] = service_path | ||
| params["jwt_expiration"] = getattr(settings, "RESOURCE_SYNC_JWT_EXPIRATION", 60) |
Contributor
There was a problem hiding this comment.
Validate RESOURCE_SYNC_JWT_EXPIRATION before passing it to the client.
Right now any setting value is forwarded as-is. Non-integer or non-positive values can cause token-generation/auth failures at runtime and break sync jobs.
Suggested fix
- params["jwt_expiration"] = getattr(settings, "RESOURCE_SYNC_JWT_EXPIRATION", 60)
+ raw_jwt_expiration = getattr(settings, "RESOURCE_SYNC_JWT_EXPIRATION", 60)
+ try:
+ jwt_expiration = int(raw_jwt_expiration)
+ except (TypeError, ValueError) as exc:
+ raise ValueError("RESOURCE_SYNC_JWT_EXPIRATION must be an integer >= 1") from exc
+ if jwt_expiration < 1:
+ raise ValueError("RESOURCE_SYNC_JWT_EXPIRATION must be an integer >= 1")
+ params["jwt_expiration"] = jwt_expiration📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| params["jwt_expiration"] = getattr(settings, "RESOURCE_SYNC_JWT_EXPIRATION", 60) | |
| raw_jwt_expiration = getattr(settings, "RESOURCE_SYNC_JWT_EXPIRATION", 60) | |
| try: | |
| jwt_expiration = int(raw_jwt_expiration) | |
| except (TypeError, ValueError) as exc: | |
| raise ValueError("RESOURCE_SYNC_JWT_EXPIRATION must be an integer >= 1") from exc | |
| if jwt_expiration < 1: | |
| raise ValueError("RESOURCE_SYNC_JWT_EXPIRATION must be an integer >= 1") | |
| params["jwt_expiration"] = jwt_expiration |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ansible_base/resource_registry/tasks/sync.py` at line 112, Validate and
sanitize settings.RESOURCE_SYNC_JWT_EXPIRATION before assigning to
params["jwt_expiration"]: ensure the value from settings is an integer > 0
(coerce with int() safely), and if it's missing, non-numeric, or non-positive,
replace it with the default 60 (and optionally log/warn). Modify the assignment
around params["jwt_expiration"] in sync.py to perform this check/coercion rather
than forwarding the raw settings value.
Comment on lines
+194
to
+197
| def __init__(self, api_client: ResourceAPIClient, page_size: int | None = None): | ||
| self.api_client = api_client | ||
| self.assignments: set[AssignmentTuple] = set() | ||
| self.page_size = page_size if page_size is not None else getattr(settings, 'RESOURCE_SYNC_PAGE_SIZE', 50) |
Contributor
There was a problem hiding this comment.
Harden page-size parsing to prevent silent partial-sync mode.
RESOURCE_SYNC_PAGE_SIZE/page_size is used without validation. Invalid values (e.g., 0, negative, non-int) can make pagination calls fail; then assignment deletions are skipped due to incomplete fetch, causing persistent local/remote drift.
Suggested fix
def __init__(self, api_client: ResourceAPIClient, page_size: int | None = None):
self.api_client = api_client
self.assignments: set[AssignmentTuple] = set()
- self.page_size = page_size if page_size is not None else getattr(settings, 'RESOURCE_SYNC_PAGE_SIZE', 50)
+ raw_page_size = page_size if page_size is not None else getattr(settings, 'RESOURCE_SYNC_PAGE_SIZE', 50)
+ try:
+ resolved_page_size = int(raw_page_size)
+ except (TypeError, ValueError) as exc:
+ raise ValueError("RESOURCE_SYNC_PAGE_SIZE/page_size must be an integer >= 1") from exc
+ if resolved_page_size < 1:
+ raise ValueError("RESOURCE_SYNC_PAGE_SIZE/page_size must be an integer >= 1")
+ self.page_size = resolved_page_sizeAlso applies to: 224-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ansible_base/resource_registry/tasks/sync.py` around lines 194 - 197, The
constructor for the sync class currently accepts page_size without validation
(see __init__ and the page_size attribute) which allows 0/negative/non-int
values to silently enable partial-sync; update __init__ to coerce and validate
page_size: if page_size is not None attempt int(page_size) and if conversion
fails or the value is <= 0, fall back to a safe default from getattr(settings,
'RESOURCE_SYNC_PAGE_SIZE', 50) (also validate that setting as an int > 0 and
fall back to 50 if not); assign the validated positive int to self.page_size and
ensure the same validation logic is applied where page_size is read/used
elsewhere (referenced around the other occurrence noted) so pagination calls
never receive non-positive or non-integer values.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Allow operators to tune RESOURCE_SYNC_PAGE_SIZE and RESOURCE_SYNC_JWT_EXPIRATION via Django settings. These are read automatically by SyncExecutor and create_api_client(), so consuming services (tower, hub, EDA) pick up new values without code changes.
This change is occurring due to multiple customers having pagination issues, and also the performance impact of having a hardcoded page_size value of 50 when thousands of roles exist.
Type of Change
Self-Review Checklist
Testing Instructions
Prerequisites
Steps to Test
Expected Results
Additional Context
Required Actions
Screenshots/Logs
Summary by CodeRabbit
New Features
--page-sizeparameter to control pagination when fetching assignments.RESOURCE_SYNC_JWT_EXPIRATIONandRESOURCE_SYNC_PAGE_SIZEwith sensible defaults.Tests