ref(integrations): Clarify which endpoints gets called#113196
Conversation
This makes it clear what endpoint is going to be called.
github-actions
bot
added
the
Scope: Backend
| "integration_id": data["integration_id"], | ||
| } | ||
|
|
||
| def compare_commits( |
There was a problem hiding this comment.
This function would call two different endpoints: compare/ and sometimes commit/.
In a sense, we're not doing any comparison of commits, thus, even making the name more confusing.
| def eval_commits(client: Any) -> Sequence[Mapping[str, Any]]: | ||
| # use config name because that is kept in sync via webhooks | ||
| name = repo.config["name"] | ||
| if start_sha is None: |
There was a problem hiding this comment.
This is the difference between calling one endpoint vs the other one.
| commits = client.compare_commits(name, start_sha, end_sha) | ||
| return self._format_commits(client, name, commits) | ||
|
|
||
| def _get_installation_and_client(self, repo: Repository) -> tuple[IntegrationInstallation, Any]: |
There was a problem hiding this comment.
We're adding two new functions and they will share this logic.
| client = installation.get_client() | ||
| return installation, installation.get_client() | ||
|
|
||
| def fetch_recent_commits(self, repo: Repository, end_sha: str) -> Sequence[Mapping[str, Any]]: |
There was a problem hiding this comment.
This one focuses on getting the last 20 commits. Not an idempotent call.
|
|
||
| try: | ||
| return eval_commits(client) | ||
| commits = client.compare_commits(name, start_sha, end_sha) |
There was a problem hiding this comment.
This is an idempotent call. In the next PR I will be moving the caching logic here.
| except Exception as e: | ||
| installation.raise_error(e) | ||
|
|
||
| def compare_commits( |
There was a problem hiding this comment.
This is for backward compatibility.
| if hasattr(provider, "fetch_commits_for_compare_range"): | ||
| repo_commits = provider.fetch_commits_for_compare_range(repo, start_sha, end_sha) | ||
| else: | ||
| repo_commits = provider.compare_commits(repo, start_sha, end_sha) |
There was a problem hiding this comment.
For backward compatibility.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| repo_commits = fetch_recent_commits( | ||
| repo=repo, | ||
| provider=provider, | ||
| is_integration_repo_provider=is_integration_repo_provider, |
There was a problem hiding this comment.
Bug: The fetch_recent_commits function is missing a lifecycle parameter, which prevents the compare_commits_cache_enabled metric from being recorded when start_sha is None.
Severity: LOW
Suggested Fix
Add a lifecycle parameter to the fetch_recent_commits function signature. Pass the lifecycle object to it from fetch_commits. Inside fetch_recent_commits, add the compare_commits_cache_enabled metric to the lifecycle to restore the previous telemetry behavior.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/sentry/tasks/commits.py#L288
Potential issue: A refactoring separated commit fetching logic into
`fetch_recent_commits` and `fetch_commits_for_compare_range`. When `start_sha` is
`None`, `fetch_commits` calls `fetch_recent_commits`. However, the `lifecycle` parameter
was not added to the new `fetch_recent_commits` function. Consequently, the
`compare_commits_cache_enabled` metric is no longer recorded for this code path. This is
a regression in observability and does not impact the core functionality of fetching
commits, nor does it cause crashes or incorrect application behavior.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
I think this is sort of an anti-pattern anyway. Function calls like this shouldn't necessarily be coupled to the metrics in the top-level tasks imo.
| commits = client.get_last_commits(name, end_sha) | ||
| return self._format_commits(client, name, commits[:20]) |
There was a problem hiding this comment.
Nit: could we update the client to take a pagination size of 20 vs retrieving 30 and trimming the number down? https://docs.github.com/en/rest/commits/commits?apiVersion=2026-03-10#list-commits-on-a-repository
| *, | ||
| repo: Repository, | ||
| provider: Any, | ||
| is_integration_repo_provider: bool, |
There was a problem hiding this comment.
Can this be standardized across provider clients consistently? Mainly asking out of ignorance for GitLab and Bitbucket's APIs.
| repo_commits = fetch_recent_commits( | ||
| repo=repo, | ||
| provider=provider, | ||
| is_integration_repo_provider=is_integration_repo_provider, |
There was a problem hiding this comment.
I think this is sort of an anti-pattern anyway. Function calls like this shouldn't necessarily be coupled to the metrics in the top-level tasks imo.
armenzg
changed the title
|
@GabeVillalobos I will follow-up on your nits after this PR. |
3 participants
This makes it clear what endpoint is going to be called.