Repository navigation
Set release just before staging changes - #261
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the process of updating the Release field in spec files for both backport and rebase agents. The change moves the release update logic to a dedicated workflow step that runs after a successful build, which ensures the release is bumped only once. This is a good improvement to make the process more robust against build failures. The implementation is solid, but there is some code duplication between the backport_agent and rebase_agent for the new update_release step. I've left a comment with a suggestion to refactor this into the shared PackageUpdateStep class to improve maintainability.
| async def update_release(state): | ||
| try: | ||
| await tasks.update_release( | ||
| local_clone=state.local_clone, | ||
| package=state.package, | ||
| dist_git_branch=state.dist_git_branch, | ||
| rebase=False, | ||
| ) | ||
| except Exception as e: | ||
| logger.warning(f"Error updating release: {e}") | ||
| state.backport_result.success = False | ||
| state.backport_result.error = f"Could not update release: {e}" | ||
| return "comment_in_jira" | ||
| return "stage_changes" |
There was a problem hiding this comment.
This update_release function is nearly identical to the one in agents/rebase_agent.py. To improve maintainability and reduce code duplication, consider moving this shared logic into the PackageUpdateStep class in agents/package_update_steps.py, which seems designed for this purpose.
You could add a new static method to PackageUpdateStep like this:
# In agents/package_update_steps.py
@staticmethod
async def update_release(state, is_rebase: bool):
"""Update release just before staging changes."""
try:
await tasks.update_release(
local_clone=state.local_clone,
package=state.package,
dist_git_branch=state.dist_git_branch,
rebase=is_rebase,
)
except Exception as e:
logger.warning(f"Error updating release: {e}")
result_attr = "rebase_result" if is_rebase else "backport_result"
result_obj = getattr(state, result_attr)
result_obj.success = False
result_obj.error = f"Could not update release: {e}"
return "comment_in_jira"
return "stage_changes"Then, you could simplify this function to:
async def update_release(state):
return await PackageUpdateStep.update_release(state, is_rebase=False)A similar change would apply to rebase_agent.py.
There was a problem hiding this comment.
I'm not actually sure what PackageUpdateStep is for.
TomasTomecek
left a comment
There was a problem hiding this comment.
LGTM, very nice!! more determinism 🚀
Make sure release is bumped only once, even in case of build failures and repeated backports/rebases. Signed-off-by: Nikola Forró <nforro@redhat.com>
|
/gemini review |
1 similar comment
|
/gemini review |
|
Did Gemini die? 😅 |
There was a problem hiding this comment.
Code Review
This pull request refactors the release update logic for both the backport and rebase agents. By moving the release update to a new workflow step that executes after a successful build, it ensures that the release is only bumped once, even if the build process requires multiple attempts. The changes are well-structured, with the core logic encapsulated in a new task in agents/tasks.py and integrated cleanly into the existing workflows. The error handling is consistent with the existing codebase. Overall, this is a good improvement for the robustness of the agents.
no eyes, so probably yes :( |
Make sure release is bumped only once, even in case of build failures and repeated backports/rebases.