ART-14708: Auto-sync golang builders to layered operator branches#2814
ART-14708: Auto-sync golang builders to layered operator branches#2814rayfordj wants to merge 2 commits into
Conversation
Add golang_sync_branches support to the update-golang pipeline. When golang is bumped for an OCP version, PRs are automatically created for layered operator branches (logging, oadp, etc.) listed in group.yml vars.golang_sync_branches. Made-with: Cursor Signed-off-by: Rayford Johnson <rayfordj@users.noreply.github.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
openshift-ci-robot
added
the
jira/valid-reference
|
@rayfordj: This pull request references ART-14708 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
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:
WalkthroughThe Changes
Sequence DiagramsequenceDiagram
participant Pipeline
participant GitHub as GitHub API
participant Config as group.yml/<br/>streams.yml
participant Slack
Pipeline->>Config: Read golang_sync_branches
Config-->>Pipeline: Return branch list
loop For each branch
Pipeline->>GitHub: Fetch streams.yml from branch
GitHub-->>Pipeline: Return streams content
Pipeline->>Pipeline: Apply pullspec replacements
alt Builder matches replacement
Pipeline->>GitHub: Create PR with updated streams.yml
GitHub-->>Pipeline: PR created or 404 error
Pipeline->>Slack: Log result/error
else No match
Pipeline->>Slack: Log skipped branch
end
end
Pipeline->>Slack: Post aggregated summary thread
Slack-->>Pipeline: Confirmation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/hold |
openshift-ci
Bot
added
do-not-merge/hold
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pyartcd/pyartcd/pipelines/update_golang.py (1)
702-705: Remove unnecessaryasyncdecorator or useasyncio.to_thread()for true parallelism.
_update_layered_branchis declared asasyncbut contains noawaitstatements—it only performs synchronous PyGithub API calls. This means each coroutine runs to completion sequentially, blocking the event loop and providing no parallelism benefit.Either:
- Convert
_update_layered_branchto a regular function and wrap calls withasyncio.to_thread()to run them in the thread pool, or- Remove the
asyncdecorator if no event loop integration is needed.The first option enables true parallel GitHub operations across branches.
Suggested refactor (option 1)
results = await asyncio.gather( - *[self._update_layered_branch(b, go_version, replacements) for b in sync_branches], + *[asyncio.to_thread(self._update_layered_branch, b, go_version, replacements) for b in sync_branches], return_exceptions=True, ) - async def _update_layered_branch( + def _update_layered_branch( self, branch_name: str, go_version: str, replacements: dict[str, str] ) -> str | None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyartcd/pyartcd/pipelines/update_golang.py` around lines 702 - 705, The call to asyncio.gather is launching coroutines for _update_layered_branch which is marked async but contains no awaits, so it runs synchronously and blocks the event loop; change _update_layered_branch to a regular (non-async) function and invoke it with asyncio.to_thread in the gather call (e.g., replace self._update_layered_branch(...) with asyncio.to_thread(self._update_layered_branch, ...) so GitHub API calls run in the thread pool), or alternatively remove the async decorator and call asyncio.to_thread for true parallelism—ensure you update any references to _update_layered_branch accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyartcd/pyartcd/pipelines/update_golang.py`:
- Around line 702-719: The code currently collects exceptions from
asyncio.gather for _update_layered_branch but only logs and summarizes them,
allowing run() to continue as a success; change this by detecting any Exception
in results after the loop and raising an error (e.g., raise RuntimeError or the
first exception) with the built summary so the pipeline fails properly.
Concretely: after building summary (the summary variable and _LOGGER.info call),
if any item in results is an Exception, log the summary, post it via
self._slack_client.say_in_thread, then raise a RuntimeError(f"Layered branch
updates failed: {summary}") (or re-raise the first Exception) so run() observes
failure instead of proceeding. Ensure you reference _update_layered_branch,
results, summary and self._slack_client.say_in_thread in the change.
- Around line 596-598: The version comparison currently using string-split is
incorrect; change the logic that checks whether latest_go > previous_go to parse
both latest_go and previous_go into numeric tuples (e.g., split on '.' and map
to ints, padding shorter tuples with zeros) and compare those tuples instead,
then ensure that when the numeric comparison is True you populate the
replacements dict with replacements[latest_go] = new_latest_go and
replacements[previous_go] = latest_go so the GO_PREVIOUS/GO_LATEST remap is
applied for the main and layered branches; update the code paths that reference
latest_go, previous_go, replacements, and any helper currently performing the
string comparison to use the numeric-tuple comparison.
---
Nitpick comments:
In `@pyartcd/pyartcd/pipelines/update_golang.py`:
- Around line 702-705: The call to asyncio.gather is launching coroutines for
_update_layered_branch which is marked async but contains no awaits, so it runs
synchronously and blocks the event loop; change _update_layered_branch to a
regular (non-async) function and invoke it with asyncio.to_thread in the gather
call (e.g., replace self._update_layered_branch(...) with
asyncio.to_thread(self._update_layered_branch, ...) so GitHub API calls run in
the thread pool), or alternatively remove the async decorator and call
asyncio.to_thread for true parallelism—ensure you update any references to
_update_layered_branch accordingly.
🪄 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: Pro Plus
Run ID: ba4782a2-de7f-40d8-a375-b04c2287be75
📒 Files selected for processing (2)
pyartcd/pyartcd/pipelines/update_golang.pypyartcd/tests/pipelines/test_update_golang.py
| replacements[latest_go] = new_latest_go | ||
| if previous_go: | ||
| replacements[previous_go] = latest_go |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
cases = [("1.10.1", "1.9"), ("2.0.0", "1.25")]
for go_version, go_latest in cases:
current_expr = (
go_version.split(".")[0] >= go_latest.split(".")[0]
and go_version.split(".")[1] > go_latest.split(".")[1]
)
print(f"{go_version} vs {go_latest}: current_expr={current_expr}")
PYRepository: openshift-eng/art-tools
Length of output: 134
🏁 Script executed:
sed -n '580,620p' pyartcd/pyartcd/pipelines/update_golang.py | cat -nRepository: openshift-eng/art-tools
Length of output: 2728
🏁 Script executed:
sed -n '590,610p' pyartcd/pyartcd/pipelines/update_golang.py | cat -nRepository: openshift-eng/art-tools
Length of output: 1370
🏁 Script executed:
python - <<'PY'
cases = [("1.10.1", "1.9"), ("2.0.0", "1.25"), ("1.9", "1.10"), ("1.20", "1.20")]
for go_version, go_latest in cases:
# Original logic
original = (
go_version.split(".")[0] >= go_latest.split(".")[0]
and go_version.split(".")[1] > go_latest.split(".")[1]
)
# Suggested fix
suggested = tuple(map(int, go_version.split('.')[:2])) > tuple(map(int, go_latest.split('.')[:2]))
print(f"{go_version} vs {go_latest}: original={original}, suggested={suggested}")
PYRepository: openshift-eng/art-tools
Length of output: 248
🏁 Script executed:
python - <<'PY'
# Test edge cases for the suggested fix
cases = [
("1", "0"), # Single digit
("1.20.5", "1.20.4"), # Patch version (should compare major.minor only)
("1.20.5", "1.19"), # Patch vs no patch
]
for go_version, go_latest in cases:
try:
parts_v = list(map(int, go_version.split('.')[:2]))
parts_l = list(map(int, go_latest.split('.')[:2]))
suggested = tuple(parts_v) > tuple(parts_l)
print(f"{go_version} vs {go_latest}: suggested={suggested}")
except (ValueError, IndexError) as e:
print(f"{go_version} vs {go_latest}: ERROR - {e}")
PYRepository: openshift-eng/art-tools
Length of output: 154
🏁 Script executed:
# Verify the fix handles version strings without proper minor versions
python - <<'PY'
# Test what happens with version strings that don't have 2 components
test_cases = [
("1", "1.0"), # What if go_version has no minor?
("1.20", "2"), # What if go_latest has no minor?
]
for go_version, go_latest in test_cases:
try:
parts_v = list(map(int, go_version.split('.')[:2]))
parts_l = list(map(int, go_latest.split('.')[:2]))
# Pad with 0 if needed
while len(parts_v) < 2:
parts_v.append(0)
while len(parts_l) < 2:
parts_l.append(0)
suggested = tuple(parts_v) > tuple(parts_l)
print(f"{go_version} vs {go_latest}: parts_v={parts_v}, parts_l={parts_l}, suggested={suggested}")
except (ValueError, IndexError) as e:
print(f"{go_version} vs {go_latest}: ERROR - {e}")
PYRepository: openshift-eng/art-tools
Length of output: 184
Fix version comparison logic to use numeric tuple comparison.
The current string-split comparison at line 585 fails to detect legitimate major-version Go upgrades. For example, 1.10.1 vs 1.9 and 2.0.0 vs 1.25 both incorrectly return False. When the condition is False, the replacements map at lines 596-598 is never populated, causing the GO_PREVIOUS/GO_LATEST remap to be skipped for both the main branch and layered branches.
Use numeric tuple comparison to fix this:
Suggested fix
- elif go_version.split('.')[0] >= go_latest.split('.')[0] and go_version.split('.')[1] > go_latest.split('.')[1]:
+ elif tuple(map(int, go_version.split('.')[:2])) > tuple(map(int, go_latest.split('.')[:2])):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyartcd/pyartcd/pipelines/update_golang.py` around lines 596 - 598, The
version comparison currently using string-split is incorrect; change the logic
that checks whether latest_go > previous_go to parse both latest_go and
previous_go into numeric tuples (e.g., split on '.' and map to ints, padding
shorter tuples with zeros) and compare those tuples instead, then ensure that
when the numeric comparison is True you populate the replacements dict with
replacements[latest_go] = new_latest_go and replacements[previous_go] =
latest_go so the GO_PREVIOUS/GO_LATEST remap is applied for the main and layered
branches; update the code paths that reference latest_go, previous_go,
replacements, and any helper currently performing the string comparison to use
the numeric-tuple comparison.
| results = await asyncio.gather( | ||
| *[self._update_layered_branch(b, go_version, replacements) for b in sync_branches], | ||
| return_exceptions=True, | ||
| ) | ||
|
|
||
| summary_lines = [] | ||
| for branch_name, result in zip(sync_branches, results): | ||
| if isinstance(result, Exception): | ||
| summary_lines.append(f" - {branch_name}: :x: error: {result}") | ||
| _LOGGER.error("Failed to update layered branch %s: %s", branch_name, result) | ||
| elif result is None: | ||
| summary_lines.append(f" - {branch_name}: skipped (no matching builders)") | ||
| else: | ||
| summary_lines.append(f" - {branch_name}: {result}") | ||
|
|
||
| summary = f"Layered branch PRs for golang {go_version}:\n" + "\n".join(summary_lines) | ||
| _LOGGER.info(summary) | ||
| await self._slack_client.say_in_thread(summary) |
There was a problem hiding this comment.
Don't mark the pipeline successful when layered sync failed.
Exceptions are summarized and then discarded here. That lets run() continue and later post the final success message even when one or more layered-branch PRs were not created.
💡 Suggested fix
- summary_lines = []
+ summary_lines = []
+ errors = []
for branch_name, result in zip(sync_branches, results):
if isinstance(result, Exception):
+ errors.append(f"{branch_name}: {result}")
summary_lines.append(f" - {branch_name}: :x: error: {result}")
_LOGGER.error("Failed to update layered branch %s: %s", branch_name, result)
elif result is None:
summary_lines.append(f" - {branch_name}: skipped (no matching builders)")
else:
summary_lines.append(f" - {branch_name}: {result}")
summary = f"Layered branch PRs for golang {go_version}:\n" + "\n".join(summary_lines)
_LOGGER.info(summary)
await self._slack_client.say_in_thread(summary)
+ if errors:
+ raise RuntimeError("Failed to update layered branches:\n" + "\n".join(errors))🧰 Tools
🪛 Ruff (0.15.10)
[warning] 708-708: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyartcd/pyartcd/pipelines/update_golang.py` around lines 702 - 719, The code
currently collects exceptions from asyncio.gather for _update_layered_branch but
only logs and summarizes them, allowing run() to continue as a success; change
this by detecting any Exception in results after the loop and raising an error
(e.g., raise RuntimeError or the first exception) with the built summary so the
pipeline fails properly. Concretely: after building summary (the summary
variable and _LOGGER.info call), if any item in results is an Exception, log the
summary, post it via self._slack_client.say_in_thread, then raise a
RuntimeError(f"Layered branch updates failed: {summary}") (or re-raise the first
Exception) so run() observes failure instead of proceeding. Ensure you reference
_update_layered_branch, results, summary and self._slack_client.say_in_thread in
the change.
Signed-off-by: Rayford Johnson <rayfordj@users.noreply.github.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
|
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. |
openshift-ci
Bot
added
the
needs-rebase
2 participants
Add golang_sync_branches support to the update-golang pipeline. When golang is bumped for an OCP version, PRs are automatically created for layered operator branches (logging, oadp, etc.) listed in group.yml vars.golang_sync_branches.
Made-with: Cursor
Signed-off-by: Rayford Johnson rayfordj@users.noreply.github.com
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED