Conversation
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
WalkthroughThe status analysis gatherer adds ChangesStatus Analysis Data Gathering
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🔵 Low · up to The PR is mergeable with owner awareness that unrelated plugin and documentation version changes should be verified against their source manifests and reverted if unintended. Sequence Diagram(s)sequenceDiagram
participant CLI
participant GatherStatusData
participant Jira
participant GitHubGraphQL
CLI->>GatherStatusData: pass updated_since_only
GatherStatusData->>Jira: search filtered roots with expanded changelogs
Jira-->>GatherStatusData: roots and changelogs
GatherStatusData->>Jira: query descendants in BFS batches
Jira-->>GatherStatusData: descendant issues
GatherStatusData->>GitHubGraphQL: request lightweight PR fields
GitHubGraphQL-->>GatherStatusData: merge timestamps
GatherStatusData->>GitHubGraphQL: request full fields for selected PRs
GitHubGraphQL-->>GatherStatusData: detailed PR data
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: celebdor, kevinrizza The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
plugins/jira/skills/status-analysis/scripts/gather_status_data.py (2)
1079-1086: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe BFS level cap can truncate descendants without a warning.
max_results=1000applies to a batch of up to 200 parent keys.search_issuesstops onceremaining <= 0and returns quietly, so a wide level loses children and every subtree below them. The reported run found 454 descendants, which is close enough to the cap to matter for larger projects.Compare the returned count against the cap and log a warning, or page until Jira reports no
nextPageToken.♻️ Proposed guard
+ bfs_page_limit = 1000 children = await self.jira.search_issues( jql=parent_jql, fields="key,parent", - max_results=1000, + max_results=bfs_page_limit, ) + if len(children) >= bfs_page_limit: + logger.warning( + f" Depth {depth}: hit the {bfs_page_limit}-result cap for " + f"{len(batch)} parent(s); descendants may be incomplete" + )🤖 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 `@plugins/jira/skills/status-analysis/scripts/gather_status_data.py` around lines 1079 - 1086, Update the BFS child lookup around jira.search_issues in the current-level traversal to prevent silent truncation at max_results=1000: either page through results until Jira provides no nextPageToken, or compare the returned count with JIRA_BFS_BATCH_SIZE’s request cap and emit a warning when the cap is reached. Preserve all returned descendants and ensure truncation is never silent.
1119-1123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused changelog API path.
No callers use
fetch_changelogs_batch,get_issue_changelog, orJIRA_CHANGELOG_DELAY_SECONDS. Remove them. Replace the redundantinline_changelogsfilter with a direct assignment.🤖 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 `@plugins/jira/skills/status-analysis/scripts/gather_status_data.py` around lines 1119 - 1123, Remove the unused fetch_changelogs_batch and get_issue_changelog functions and the JIRA_CHANGELOG_DELAY_SECONDS constant. In the Step 4 flow, replace the filtered inline_changelogs comprehension with a direct assignment to changelogs, preserving the existing logging and downstream behavior.
🤖 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.
Inline comments:
In `@plugins/jira/skills/status-analysis/scripts/gather_status_data.py`:
- Around line 1044-1056: Update the status-entry parsing around
DateRange.contains to retain and pass the parsed datetime rather than converting
it to a date, preserving the type used by other contains call sites. In the item
matching within the same flow, accept either the matching fieldId or the
configured field name (as used by _build_manifest), while preserving the
existing active_keys and early-break behavior.
- Around line 775-782: Update the PR pre-filter in the batch loop to retain
records whose mergedAt is within range or whose updatedAt falls within the
requested range, including OPEN PRs; preserve the existing merged_refs
collection and downstream _filter_pr_to_range behavior. Also revise the
--updated-since-only help text to document the resulting
merged-or-recently-updated PR selection.
- Around line 751-773: Update the lightweight batch request loop in the pass-1
logic surrounding _build_pr_query to retry transient HTTP 502/503/504 responses
and timeouts using the same retry count and backoff behavior as get_prs_batch.
Replace the broad Exception handler with aiohttp.ClientError and
asyncio.TimeoutError handling, preserving the existing batch logging and
continuation behavior after retries are exhausted.
- Around line 1087-1096: Update the child-processing loop around child_key and
key_to_root so the child’s parent data safely handles missing or null parent
values, resolves the root only from known parent/root mappings, and skips the
child when no root can be resolved. Perform this resolution before adding the
child to visited or next_level_keys, and only append descendants and record
mappings after a valid root is found.
- Line 1019: Update the changelog retrieval helper used by the status analysis
flow to paginate the issue changelog endpoint until all histories are collected,
rather than relying on expand="changelog" or a single page. Preserve complete
histories in oldest-first order, and reuse that ordered result for both
pre-filtering and manifest generation so last_status_summary_update is selected
correctly.
---
Nitpick comments:
In `@plugins/jira/skills/status-analysis/scripts/gather_status_data.py`:
- Around line 1079-1086: Update the BFS child lookup around jira.search_issues
in the current-level traversal to prevent silent truncation at max_results=1000:
either page through results until Jira provides no nextPageToken, or compare the
returned count with JIRA_BFS_BATCH_SIZE’s request cap and emit a warning when
the cap is reached. Preserve all returned descendants and ensure truncation is
never silent.
- Around line 1119-1123: Remove the unused fetch_changelogs_batch and
get_issue_changelog functions and the JIRA_CHANGELOG_DELAY_SECONDS constant. In
the Step 4 flow, replace the filtered inline_changelogs comprehension with a
direct assignment to changelogs, preserving the existing logging and downstream
behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 96105c9a-a1af-4e50-bfb3-145a57d9bac9
📒 Files selected for processing (1)
plugins/jira/skills/status-analysis/scripts/gather_status_data.py
| for batch_start in range(0, len(pr_refs), GITHUB_GRAPHQL_BATCH_SIZE): | ||
| batch = pr_refs[batch_start : batch_start + GITHUB_GRAPHQL_BATCH_SIZE] | ||
| batch_num = batch_start // GITHUB_GRAPHQL_BATCH_SIZE + 1 | ||
| query = self._build_pr_query(batch, lightweight=True) | ||
|
|
||
| data = None | ||
| async with self.semaphore: | ||
| try: | ||
| async with session.post( | ||
| self.GRAPHQL_URL, | ||
| headers=self._get_headers(), | ||
| json={"query": query}, | ||
| timeout=aiohttp.ClientTimeout(total=30), | ||
| ) as resp: | ||
| if resp.status == 200: | ||
| data = await resp.json() | ||
| else: | ||
| logger.warning(f" Lightweight batch {batch_num}/{total_batches}: HTTP {resp.status}") | ||
| except Exception as e: | ||
| logger.warning(f" Lightweight batch {batch_num}/{total_batches}: {e}") | ||
|
|
||
| if not data or "data" not in data: | ||
| continue |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Pass 1 drops PRs silently on transient GitHub errors.
get_prs_batch retries 502/503/504 and timeouts. Pass 1 does not. One transient failure discards up to GITHUB_GRAPHQL_BATCH_SIZE PR references, and those PRs never reach pass 2. The output then omits merged PRs with no error surfaced to the caller, which is the failure mode this PR aims to reduce.
Add the same retry and backoff to pass 1, and narrow the exception clause to aiohttp.ClientError and asyncio.TimeoutError to satisfy Ruff BLE001.
🛠️ Proposed retry for the lightweight pass
data = None
- async with self.semaphore:
- try:
- async with session.post(
- self.GRAPHQL_URL,
- headers=self._get_headers(),
- json={"query": query},
- timeout=aiohttp.ClientTimeout(total=30),
- ) as resp:
- if resp.status == 200:
- data = await resp.json()
- else:
- logger.warning(f" Lightweight batch {batch_num}/{total_batches}: HTTP {resp.status}")
- except Exception as e:
- logger.warning(f" Lightweight batch {batch_num}/{total_batches}: {e}")
+ for attempt in range(1, GITHUB_RETRY_ATTEMPTS + 1):
+ async with self.semaphore:
+ try:
+ async with session.post(
+ self.GRAPHQL_URL,
+ headers=self._get_headers(),
+ json={"query": query},
+ timeout=aiohttp.ClientTimeout(total=30),
+ ) as resp:
+ if resp.status == 200:
+ data = await resp.json()
+ break
+ retryable = resp.status in (502, 503, 504)
+ logger.warning(
+ f" Lightweight batch {batch_num}/{total_batches}: HTTP {resp.status}"
+ )
+ if not retryable:
+ break
+ except (aiohttp.ClientError, asyncio.TimeoutError) as e:
+ logger.warning(f" Lightweight batch {batch_num}/{total_batches}: {e}")
+ if attempt < GITHUB_RETRY_ATTEMPTS:
+ await asyncio.sleep(GITHUB_RETRY_DELAY_SECONDS * attempt)
+ if data is None:
+ logger.error(
+ f" Lightweight batch {batch_num}/{total_batches} failed; "
+ f"{len(batch)} PR(s) skipped"
+ )📝 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.
| for batch_start in range(0, len(pr_refs), GITHUB_GRAPHQL_BATCH_SIZE): | |
| batch = pr_refs[batch_start : batch_start + GITHUB_GRAPHQL_BATCH_SIZE] | |
| batch_num = batch_start // GITHUB_GRAPHQL_BATCH_SIZE + 1 | |
| query = self._build_pr_query(batch, lightweight=True) | |
| data = None | |
| async with self.semaphore: | |
| try: | |
| async with session.post( | |
| self.GRAPHQL_URL, | |
| headers=self._get_headers(), | |
| json={"query": query}, | |
| timeout=aiohttp.ClientTimeout(total=30), | |
| ) as resp: | |
| if resp.status == 200: | |
| data = await resp.json() | |
| else: | |
| logger.warning(f" Lightweight batch {batch_num}/{total_batches}: HTTP {resp.status}") | |
| except Exception as e: | |
| logger.warning(f" Lightweight batch {batch_num}/{total_batches}: {e}") | |
| if not data or "data" not in data: | |
| continue | |
| for batch_start in range(0, len(pr_refs), GITHUB_GRAPHQL_BATCH_SIZE): | |
| batch = pr_refs[batch_start : batch_start + GITHUB_GRAPHQL_BATCH_SIZE] | |
| batch_num = batch_start // GITHUB_GRAPHQL_BATCH_SIZE + 1 | |
| query = self._build_pr_query(batch, lightweight=True) | |
| data = None | |
| for attempt in range(1, GITHUB_RETRY_ATTEMPTS + 1): | |
| async with self.semaphore: | |
| try: | |
| async with session.post( | |
| self.GRAPHQL_URL, | |
| headers=self._get_headers(), | |
| json={"query": query}, | |
| timeout=aiohttp.ClientTimeout(total=30), | |
| ) as resp: | |
| if resp.status == 200: | |
| data = await resp.json() | |
| break | |
| retryable = resp.status in (502, 503, 504) | |
| logger.warning( | |
| f" Lightweight batch {batch_num}/{total_batches}: HTTP {resp.status}" | |
| ) | |
| if not retryable: | |
| break | |
| except (aiohttp.ClientError, asyncio.TimeoutError) as e: | |
| logger.warning(f" Lightweight batch {batch_num}/{total_batches}: {e}") | |
| if attempt < GITHUB_RETRY_ATTEMPTS: | |
| await asyncio.sleep(GITHUB_RETRY_DELAY_SECONDS * attempt) | |
| if data is None: | |
| logger.error( | |
| f" Lightweight batch {batch_num}/{total_batches} failed; " | |
| f"{len(batch)} PR(s) skipped" | |
| ) | |
| if not data or "data" not in data: | |
| continue |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 769-769: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@plugins/jira/skills/status-analysis/scripts/gather_status_data.py` around
lines 751 - 773, Update the lightweight batch request loop in the pass-1 logic
surrounding _build_pr_query to retry transient HTTP 502/503/504 responses and
timeouts using the same retry count and backoff behavior as get_prs_batch.
Replace the broad Exception handler with aiohttp.ClientError and
asyncio.TimeoutError handling, preserving the existing batch logging and
continuation behavior after retries are exhausted.
Source: Linters/SAST tools
| for i, ref in enumerate(batch): | ||
| pr_data = (data["data"].get(f"pr{i}") or {}).get("pullRequest") | ||
| if pr_data and pr_data.get("state") == "MERGED": | ||
| merged_at = pr_data.get("mergedAt", "") | ||
| if merged_at and merged_at >= merged_since: | ||
| merged_refs.append(ref) | ||
|
|
||
| logger.info(f" {len(merged_refs)} PRs merged since {merged_since} (out of {len(pr_refs)} total)") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The two-pass filter excludes all open PRs, not just inactive ones.
Pass 1 keeps a PR only when state == "MERGED" and mergedAt >= merged_since. Every OPEN PR is therefore dropped before pass 2, including PRs with commits, reviews, or review comments inside the date range. _filter_pr_to_range is built to report exactly that in-range activity, so --updated-since-only changes report content, not only runtime.
Keep PRs whose updatedAt falls in range as well, so in-flight work still appears. If merged-only is intentional, state it in the --updated-since-only help text at Line 1505, because the current text describes Jira pre-filtering only.
🔍 Proposed filter that also keeps recently updated PRs
if lightweight:
fields = """
url
state
+ updatedAt
mergedAt""" for i, ref in enumerate(batch):
pr_data = (data["data"].get(f"pr{i}") or {}).get("pullRequest")
- if pr_data and pr_data.get("state") == "MERGED":
- merged_at = pr_data.get("mergedAt", "")
- if merged_at and merged_at >= merged_since:
- merged_refs.append(ref)
+ if not pr_data:
+ continue
+ merged_at = pr_data.get("mergedAt") or ""
+ updated_at = pr_data.get("updatedAt") or ""
+ if merged_at >= merged_since or updated_at >= merged_since:
+ merged_refs.append(ref)🤖 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 `@plugins/jira/skills/status-analysis/scripts/gather_status_data.py` around
lines 775 - 782, Update the PR pre-filter in the batch loop to retain records
whose mergedAt is within range or whose updatedAt falls within the requested
range, including OPEN PRs; preserve the existing merged_refs collection and
downstream _filter_pr_to_range behavior. Also revise the --updated-since-only
help text to document the resulting merged-or-recently-updated PR selection.
| root_issues = await self.jira.search_issues( | ||
| jql=jql, | ||
| fields=f"{self.jira.ISSUE_FIELDS},{self.config.status_summary_field}", | ||
| expand="changelog", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Jira Cloud search jql expand changelog truncated maximum histories returned limit ordering
💡 Result:
In Jira Cloud, when using the expand=changelog parameter in issue search endpoints (such as the JQL search API), the number of changelog history records returned is hard-limited [1][2][3]. While older documentation and community reports historically cited a limit of 100 [1][4], users have more recently reported and observed a stricter limit of 40 records [2][5]. This limit is applied to the changelog expansion independently of the maxResults parameter used for the issue search itself [2][3], and there is no supported way to increase this limit or paginate changelog results through the search endpoint [6][3]. For scenarios requiring access to the full issue history, Atlassian recommends using the dedicated issue changelog endpoint [1][5][3]: GET /rest/api/3/issue/{issueIdOrKey}/changelog This dedicated endpoint supports standard pagination using startAt and maxResults parameters [1][6], allowing you to retrieve the complete history for an issue [1][4]. Regarding ordering, when using expand=changelog in the search API, the returned changelogs are typically sorted in descending order (most recent first) to ensure the most relevant updates are provided within the limited result set [4]. In contrast, the dedicated /issue/{issueIdOrKey}/changelog endpoint defaults to ascending order (oldest first) [1][4]. For bulk operations, you can also utilize the bulk fetch changelogs API [7]: POST /rest/api/3/changelog/bulkfetch This endpoint allows you to retrieve paginated changelogs for multiple issues simultaneously, sorted by changelog date and issue ID [7].
Citations:
- 1: https://community.atlassian.com/forums/Jira-questions/Rest-API-limiting-changelog-history-results-to-100-even-if/qaq-p/1466525
- 2: https://community.atlassian.com/forums/Jira-questions/Changlog-history-records-are-limited-to-40-records-only-with/qaq-p/3144974
- 3: https://jira.atlassian.com/browse/JRACLOUD-79536
- 4: https://community.developer.atlassian.com/t/changelogs-sorting-limitation-rest-api/8620
- 5: https://jira.atlassian.com/browse/JRACLOUD-96459
- 6: https://community.atlassian.com/forums/Jira-questions/How-to-get-changelogs-for-an-entire-project-using-the-search-api/qaq-p/1980217
- 7: https://developer.atlassian.com/cloud/jira/platform/rest/v3/api-group-issues/
🏁 Script executed:
#!/bin/bash
set -eu
file=$(git ls-files | grep -F 'plugins/jira/skills/status-analysis/scripts/gather_status_data.py' | head -n 1)
printf '%s\n' "$file"
sed -n '990,1070p' "$file"
sed -n '1300,1340p' "$file"
rg -n -C 3 'fetch_changelogs_batch|get_issue_changelog|issue_changelog|last_status_summary_update|changelog_in_range' "$file"Repository: openshift-eng/ai-helpers
Length of output: 8471
🏁 Script executed:
#!/bin/bash
set -eu
file=plugins/jira/skills/status-analysis/scripts/gather_status_data.py
sed -n '1070,1150p' "$file"
sed -n '1230,1325p' "$file"
sed -n '380,455p' "$file"
rg -n -C 5 'changelogs|_build_manifest|_filter_changelog_to_range|search_issues' "$file"Repository: openshift-eng/ai-helpers
Length of output: 19534
🌐 Web query:
site:developer.atlassian.com/cloud/jira/platform/rest/v3/api-group-issue-search expand changelog 40 ordering search/jql
💡 Result:
There is no JQL field or operator named changelog, and therefore no support for ordering or searching by a changelog field in JQL. Jira JQL is used to search for issues based on their properties and metadata, not to perform queries against individual issue change history logs directly. While the Jira REST API allows for the retrieval of issue change history, this functionality is not exposed as a queryable field within JQL [1][2].
Citations:
- 1: https://developer.atlassian.com/cloud/jira/platform/rest/v3/api-group-issue-search/
- 2: https://developer.atlassian.com/cloud/jira/platform/rest/v3/api-group-issue-search
Fetch complete root changelogs through the paginated issue changelog endpoint.
expand=changelog is capped at 40 histories, regardless of max_results, and returns newest-first. This can omit in-range changes and makes reversed(issue_changelog) select the wrong last_status_summary_update. Paginate /rest/api/3/issue/{issueKey}/changelog and use the complete, oldest-first histories for both the pre-filter and manifest generation. The current helper requests only one page.
🤖 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 `@plugins/jira/skills/status-analysis/scripts/gather_status_data.py` at line
1019, Update the changelog retrieval helper used by the status analysis flow to
paginate the issue changelog endpoint until all histories are collected, rather
than relying on expand="changelog" or a single page. Preserve complete histories
in oldest-first order, and reuse that ordered result for both pre-filtering and
manifest generation so last_status_summary_update is selected correctly.
| entry_date = None | ||
| if created: | ||
| try: | ||
| entry_date = datetime.fromisoformat(created.replace("Z", "+00:00")).date() | ||
| except (ValueError, TypeError): | ||
| pass | ||
| if entry_date and self.config.date_range.contains(entry_date): | ||
| for item in entry.get("items", []): | ||
| if item.get("fieldId") == self.config.status_summary_field: | ||
| active_keys.add(key) | ||
| break | ||
| if key in active_keys: | ||
| break |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Confirm DateRange.contains accepts a date, and add a field-name fallback.
Line 1047 converts to .date() and Line 1050 passes that date to contains. All other call sites pass a datetime, for example Line 858 and Line 1295. If contains compares against datetime bounds or calls .date() on its argument, this call either raises TypeError or shifts the boundary days. Passing the parsed datetime directly keeps this call consistent with the rest of the file.
Line 1052 matches only item.get("fieldId"). _build_manifest at Lines 1326-1327 matches the field name instead. If fieldId is missing from an entry, active_keys stays empty and the run exits at Line 1063 with a message that suggests no activity rather than a matching gap. Match both keys.
🛠️ Proposed fix
- entry_date = None
+ entry_dt = None
if created:
try:
- entry_date = datetime.fromisoformat(created.replace("Z", "+00:00")).date()
+ entry_dt = datetime.fromisoformat(created.replace("Z", "+00:00"))
except (ValueError, TypeError):
pass
- if entry_date and self.config.date_range.contains(entry_date):
+ if entry_dt and self.config.date_range.contains(entry_dt):
for item in entry.get("items", []):
- if item.get("fieldId") == self.config.status_summary_field:
+ field_name = (item.get("field") or "").lower()
+ if (
+ item.get("fieldId") == self.config.status_summary_field
+ or ("status" in field_name and "summary" in field_name)
+ ):
active_keys.add(key)
breakRun the following script to confirm the accepted argument type:
#!/bin/bash
# Description: Inspect the DateRange definition and every contains() call site.
set -euo pipefail
fd -t f 'gather_status_data.py' --exec ast-grep outline {} --items all
fd -t f 'gather_status_data.py' --exec rg -n -C 6 'class DateRange' {}
fd -t f 'gather_status_data.py' --exec rg -nP -C 2 '\bcontains\s*\(' {}🤖 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 `@plugins/jira/skills/status-analysis/scripts/gather_status_data.py` around
lines 1044 - 1056, Update the status-entry parsing around DateRange.contains to
retain and pass the parsed datetime rather than converting it to a date,
preserving the type used by other contains call sites. In the item matching
within the same flow, accept either the matching fieldId or the configured field
name (as used by _build_manifest), while preserving the existing active_keys and
early-break behavior.
| for child in children: | ||
| child_key = child["key"] | ||
| if child_key not in visited: | ||
| visited.add(child_key) | ||
| descendant_keys.append(child_key) | ||
| queue.append(child_key) | ||
| all_descendant_keys[issue_key] = descendant_keys | ||
| logger.debug(f" {issue_key} has {len(descendant_keys)} descendants") | ||
| next_level_keys.append(child_key) | ||
| # Determine root: parent's root is our root | ||
| parent_key = child.get("fields", {}).get("parent", {}).get("key", "") | ||
| root = key_to_root.get(parent_key, parent_key) | ||
| key_to_root[child_key] = root | ||
| all_descendant_keys[root].append(child_key) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unresolved parent keys raise KeyError and abort the gather.
Line 1093 defaults to "" when the parent field is absent, and Line 1094 then falls back to root = parent_key. That fallback value is not a key of all_descendant_keys, which is seeded with root keys only at Line 1067, so Line 1096 raises KeyError. Line 1093 also raises AttributeError when fields contains parent: null, because the {} default applies only to a missing key.
Resolve the root before you mark the child visited, and skip children whose root cannot be resolved.
🐛 Proposed fix
for child in children:
child_key = child["key"]
- if child_key not in visited:
- visited.add(child_key)
- next_level_keys.append(child_key)
- # Determine root: parent's root is our root
- parent_key = child.get("fields", {}).get("parent", {}).get("key", "")
- root = key_to_root.get(parent_key, parent_key)
- key_to_root[child_key] = root
- all_descendant_keys[root].append(child_key)
+ if child_key in visited:
+ continue
+ # Determine root: parent's root is our root
+ parent_key = ((child.get("fields") or {}).get("parent") or {}).get("key", "")
+ root = key_to_root.get(parent_key)
+ if root is None:
+ logger.warning(
+ f" Skipping {child_key}: parent {parent_key!r} has no known root"
+ )
+ continue
+ visited.add(child_key)
+ next_level_keys.append(child_key)
+ key_to_root[child_key] = root
+ all_descendant_keys[root].append(child_key)📝 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.
| for child in children: | |
| child_key = child["key"] | |
| if child_key not in visited: | |
| visited.add(child_key) | |
| descendant_keys.append(child_key) | |
| queue.append(child_key) | |
| all_descendant_keys[issue_key] = descendant_keys | |
| logger.debug(f" {issue_key} has {len(descendant_keys)} descendants") | |
| next_level_keys.append(child_key) | |
| # Determine root: parent's root is our root | |
| parent_key = child.get("fields", {}).get("parent", {}).get("key", "") | |
| root = key_to_root.get(parent_key, parent_key) | |
| key_to_root[child_key] = root | |
| all_descendant_keys[root].append(child_key) | |
| for child in children: | |
| child_key = child["key"] | |
| if child_key in visited: | |
| continue | |
| # Determine root: parent's root is our root | |
| parent_key = ((child.get("fields") or {}).get("parent") or {}).get("key", "") | |
| root = key_to_root.get(parent_key) | |
| if root is None: | |
| logger.warning( | |
| f" Skipping {child_key}: parent {parent_key!r} has no known root" | |
| ) | |
| continue | |
| visited.add(child_key) | |
| next_level_keys.append(child_key) | |
| key_to_root[child_key] = root | |
| all_descendant_keys[root].append(child_key) |
🤖 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 `@plugins/jira/skills/status-analysis/scripts/gather_status_data.py` around
lines 1087 - 1096, Update the child-processing loop around child_key and
key_to_root so the child’s parent data safely handles missing or null parent
values, resolves the root only from known parent/root mappings, and skips the
child when no root can be resolved. Perform this resolution before adding the
child to visited or next_level_keys, and only append descendants and record
mappings after a valid root is found.
|
/uncc |
- Reduce GITHUB_GRAPHQL_BATCH_SIZE from 30 to 10 to avoid persistent
GitHub 502 errors on large PR batch queries
- Add two-pass PR fetching: lightweight filter (url/state/mergedAt)
then heavy fetch for merged PRs only, drastically reducing payload
- Replace per-root sequential BFS with level-by-level batched BFS using
parent in (...) queries, reducing API calls from O(nodes) to O(depth)
- Fetch changelogs inline via expand=changelog in the initial search
instead of a separate batch, saving one API round-trip per root issue
- Add --updated-since-only flag to pre-filter root issues whose Status
Summary field did not change in the date range
- Fix expand parameter format (string not array) for Jira Cloud v3 API
- Fix GraphQL template syntax ({{ vs { in non-f-string contexts)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
52315e5 to
8cf9333
Compare
|
New changes are detected. LGTM label has been removed. |
Review: Performance optimizations for gather_status_dataNice set of optimizations — the batched level-by-level BFS is a particularly clever redesign that collapses ~588 API calls down to ~3-4 for a typical tree. Some suggestions below, roughly ordered by impact. 1.
|
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
the
lifecycle/stale
4 participants
Summary
GITHUB_GRAPHQL_BATCH_SIZEfrom 30 to 10 to avoid persistent GitHub 502 errors on large PR batch queriesparent in (...)queries, reducing Jira API calls from O(nodes) to O(depth × batch-count)expand=changelogin the initial search instead of a separate batch, saving one API round-trip per root issue--updated-since-onlyflag to pre-filter root issues whose Status Summary field did not change in the date rangeexpandparameter format (string not array) for Jira Cloud v3 API{{vs{in non-f-string contexts)Test plan
gather_status_data.py --project OCPSTRAT --label control-plane-work --component "Hosted Control Planes" --updated-since-only --debugsuccessfully (20 issues, 454 descendants, 12 PRs in 39s)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Performance Improvements
Maintenance