Fix exception handling in StatusReporterGitlab.set_status #2922
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses an exception handling issue in set_status when falling back to posting a comment. The new logic, which checks for a commit SHA, then a pull request ID, before logging a warning, is a robust improvement and has been correctly applied to the GitHub, GitLab, and Pagure reporters. The accompanying test changes are thorough and correctly validate the new behavior.
I have one suggestion to refactor duplicated exception handling logic into the base class to improve maintainability. Please see my comment.
| if self.commit_sha: | ||
| logger.debug( | ||
| f"Failed to set status for {self.commit_sha}," | ||
| f" commenting on commit as a fallback: {e}", | ||
| ) | ||
| self._add_commit_comment_with_status(state, description, check_name, url) | ||
| elif self.pr_id: | ||
| logger.debug( | ||
| f"Failed to set status and no commit SHA available," | ||
| f" commenting on PR {self.pr_id} as a fallback: {e}", | ||
| ) | ||
| self.report_status_by_comment( | ||
| state, | ||
| url, | ||
| check_name, | ||
| description, | ||
| ) | ||
| else: | ||
| logger.warning( | ||
| f"Failed to set status and cannot comment as a fallback," | ||
| f" no commit SHA and no PR id: {e}", | ||
| ) |
There was a problem hiding this comment.
This exception handling logic is duplicated in packit_service/worker/reporting/reporters/gitlab.py. To improve maintainability and reduce code duplication, consider moving this logic to a new method in the StatusReporter base class in packit_service/worker/reporting/reporters/base.py.
For example, you could add a method like _handle_set_status_failure to StatusReporter:
# in packit_service/worker/reporting/reporters/base.py
def _handle_set_status_failure(self, e, state, description, check_name, url):
if self.commit_sha:
logger.debug(
f"Failed to set status for {self.commit_sha},"
f" commenting on commit as a fallback: {e}",
)
self._add_commit_comment_with_status(state, description, check_name, url)
elif self.pr_id:
logger.debug(
f"Failed to set status and no commit SHA available,"
f" commenting on PR {self.pr_id} as a fallback: {e}",
)
self.report_status_by_comment(
state,
url,
check_name,
description,
)
else:
logger.warning(
f"Failed to set status and cannot comment as a fallback,"
f" no commit SHA and no PR id: {e}",
)Then you could call this helper from both StatusReporterGithubStatuses and StatusReporterGitlab.
There was a problem hiding this comment.
Good idea, I've implemented it.
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 47s |
Post a comment on the commit only when the commit SHA is available. Otherwise, post on the pull request; if the pull request ID is also unavailable, log a warning. Resolves: packit#2838 Signed-off-by: Marek Blaha <mblaha@redhat.com>
Signed-off-by: Marek Blaha <mblaha@redhat.com>
m-blaha
force-pushed
the
commit-none-pr
branch
from
68393a0 to
07d462e
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 47s |
1 participant
Post a comment on the commit only when the commit SHA is available. Otherwise, post on the pull request; if the pull request ID is also unavailable, log a warning.
Resolves issue when GitlabAPIException was raised from exception handler preventing CoprBuildEndHandler to finish correctly.
Resolves: #2838