Repository navigation
Add consolidation label processing to TODO fetcher - #760
Conversation
PR Summary by QodoInclude consolidation labels in TODO fetcher JQL for faster job submission
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1.
|
bee14f3 to
861fd71
Compare
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 861fd71 |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 03c7c49 |
ab143c7 to
d806d5c
Compare
Add ymir_consolidate_base and ymir_consolidate_next labels to the TODO fetcher query (runs every 5 minutes) for faster consolidation job submission. This complements the existing daily fetcher consolidation processing, providing a faster path when users manually add consolidation labels. Consolidation is a manual operation where someone adds labels to two issues to trigger MR consolidation. The daily fetcher (8am UTC) still processes these labels, but with the TODO fetcher also checking them, the maximum delay drops from 24 hours to 5 minutes. The _process_consolidation_labels() method is already called by both fetchers in the same run() method, so no code changes needed - just include the labels in the query. Impact: - Faster response: Up to 5min delay instead of up to 24h - Redundant processing: Both fetchers will check, but deduplication in submit_merge_job() prevents duplicate submissions - Overhead: Negligible (label scanning on already-fetched issues) Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Add Red Hat Employee verification for ymir_consolidate_base and ymir_consolidate_next labels to prevent untrusted external collaborators from submitting MR consolidation jobs. Changes: 1. Make _label_added_by_rh_employee() generic by adding optional label parameter (defaults to ymir_todo for backwards compatibility) 2. Add verification in _process_consolidation_labels() before processing each issue - checks both base and next labels independently 3. Remove labels that fail verification (same cleanup pattern as ymir_todo) 4. Skip issues on transient errors to avoid false positives Security context: - Consolidation labels now included in TODO fetcher (every 5 min) - Without verification, external collaborators could trigger jobs - ymir_todo already has this protection, consolidation labels did not Behavior: - If label not added by RH employee: skip issue, remove label, log warning - If verification fails transiently: skip issue this sweep, retry next time - If verification succeeds: proceed with consolidation as before Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
When submit_merge_job() returns False (pending job already exists), skip posting the "job submitted" comment to avoid misleading users. This is now more likely with two cronjobs (daily + TODO every 5min) processing the same consolidation labels. Changes: - Only post "job submitted" comment when result is True - Still remove labels in both cases (prevents re-processing) - Log "removing labels without commenting" when job already queued - Refactor comment posting to loop over [base_key, next_key] for clarity Before: Posted "job submitted" even when job was already queued After: Only posts comment when job is newly submitted Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Update the OpenShift README to document that the TODO fetcher now processes consolidation labels in addition to ymir_todo. Changes: - Update table to show TODO fetcher processes ymir_todo OR consolidation labels - Add detailed explanation of what the TODO fetcher processes: - User-triggered issues (ymir_todo) - Consolidation requests (ymir_consolidate_base + ymir_consolidate_next) - Note that both fetchers process consolidation labels (daily as fallback) - Reference the ConfigMap for exact JQL query Before: README said TODO fetcher only processes `labels = "ymir_todo"` After: Documents the actual query including consolidation labels Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Change _label_added_by_rh_employee() to re-raise 401/403 errors instead of returning False, preventing valid consolidation labels from being deleted during Jira auth/permission outages. Problem: - 401/403 were converted to False (treat as non-RH-employee) - Callers removed labels when verification returned False - Jira auth outage → all consolidation labels deleted → lost requests Solution: - Re-raise 401/403 as transient errors - Existing RequestException handlers skip without label removal - Only remove labels on 400/404 (legitimate verification failure) Error handling: - 401/403: Auth/permission - re-raise (transient, retry next sweep) - 400/404: Bad request/not found - return False (verification failed) - Other HTTP: Re-raise (transient) - Parse errors: Return False (verification failed) This matches the existing TODO label verification pattern where transient errors are caught and skipped without label removal. Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
d806d5c to
61d44f7
Compare
| ) | ||
| continue | ||
|
|
||
| if not is_rh_employee: |
There was a problem hiding this comment.
nice! we can mark this as mitigated in the threat model
There was a problem hiding this comment.
Oh I thought this was already mentioned in the threat model document. I remember a comment about it. But I would double check.
Summary
Add
ymir_consolidate_baseandymir_consolidate_nextlabels to the TODO fetcher query for faster consolidation job submission (5 minutes instead of up to 24 hours).Change
Updated TODO fetcher JQL query to include consolidation labels:
Impact
submit_merge_job()deduplication prevents duplicate submissions_process_consolidation_labels()already called by both fetchersBackground
Consolidation is a manual operation where users add labels to two issues to trigger MR consolidation. The daily fetcher (8am UTC) already processes these labels, but this adds a faster path through the TODO fetcher which runs every 5 minutes.