feat(codecov): initial client for requests to codecov api #91830
Conversation
github-actions
bot
added
the
Scope: Backend
matt-codecov
force-pushed
the
mhammerly/feat/codecov-client
branch
from
14579a4 to
9a323ff
Compare
|
| API host with the provided params and headers. | ||
|
|
||
| :param endpoint: The endpoint to request, without the host portion. For | ||
| examples: `/api/v2/gh/getsentry/users` or `/graphql` |
There was a problem hiding this comment.
nit: example vs examples
| API host with the provided data and headers. | ||
|
|
||
| :param endpoint: The endpoint to request, without the host portion. For | ||
| examples: `/api/v2/gh/getsentry/users` or `/graphql` |
There was a problem hiding this comment.
same comments here for example and the log
| if not (signing_secret := options.get("codecov.api-bridge-signing-secret")): | ||
| raise ConfigurationError() |
There was a problem hiding this comment.
We're trying to not store secrets in runtime options, as they are stored in plain text everywhere (code + db). Instead could we use a django setting + environment var for this? That will allow us to put secrets in encrypted storage.
There was a problem hiding this comment.
this actually is populated by an environment variable in https://github.com/getsentry/getsentry/pull/17508, will that work?
| hosted on. | ||
| """ | ||
|
|
||
| if not (base_url := options.get("codecov.base-url")): |
There was a problem hiding this comment.
This could likely also be a django setting, as it is infrastructure plumbing and wouldn't be manipulated at runtime.
There was a problem hiding this comment.
there's a way to override django settings for local development, i imagine? i'll look into that, ty
| try: | ||
| response = requests.post(url, data=data, headers=headers, timeout=TIMEOUT_SECONDS) | ||
| except Exception: | ||
| logger.exception("Error when making POST request") | ||
| return None |
There was a problem hiding this comment.
Thoughts about capturing server errors and retrying?
| response = requests.get(url, params=params, headers=headers, timeout=TIMEOUT_SECONDS) | ||
| except Exception: | ||
| logger.exception("Error when making GET request") | ||
| return None |
There was a problem hiding this comment.
I think we will probably want to differentiate a 400 vs 500 response from Codecov in the API response. They should be handled differently in the Sentry REST API.
There was a problem hiding this comment.
Ah, just realized that you would need a raise_for_status to actually raise the status exceptions. Anyways, we can either handle the errors here in the client, which might be easier for standardization, or leave it to the caller to handle.
|
|
||
| def _create_jwt(self): | ||
| now = int(datetime.datetime.now(datetime.UTC).timestamp()) | ||
| exp = now + 300 # 5 minutes |
There was a problem hiding this comment.
nit: Let's make 300 a constant (with a descriptive name like FIVE_MINUTES)
TODO: make this work for monolith mode, i didn't consider monolith mode when I made these changes. I think the path forward for that is to define another middleware that takes care of that and only operates in monolith mode. depends on: #91830 We want to forward webhooks Sentry receives to Codecov since Codecov depends on receiving webhooks from the code forges it supports. We're starting with only GitHub webhooks for now, but plan to support more in the future. We're using the existing WebhookPayload system to forward webhooks to Codecov by creating an additional WebhookPayload object in the GithubRequestParser. Codecov will do additional filtering on its end to filter out irrelevant webhooks so we don't have to do any filtering on Sentry's end, this is to avoid keeping track of what's relevant to Codecov in Sentry. I'm adding a destination_type column to the WebhookPayload so we can distinguish between payloads meant for Codecov and other Sentry regions because when we make the request in the drain_mailbox we want to run different logic, using the CodecovApiClient. I also made the choice to use a different mailbox_name so that the forwarding doesn't interfere with the webhooks being sent to the region silos. Finally, because we want to forward ALL webhooks to Codecov, that means we're forwarding installation events, a webhook that in Sentry gets processed in the control silo, so that gets its own mailbox identifier "installation".
Forwarding webhooks for Sentry running in Monolith mode is considered out of scope for this change depends on: #91830 We want to forward webhooks Sentry receives to Codecov since Codecov depends on receiving webhooks from the code forges it supports. We're starting with only GitHub webhooks for now, but plan to support more in the future. We're using the existing WebhookPayload system to forward webhooks to Codecov by creating an additional WebhookPayload object in the GithubRequestParser. Codecov will do additional filtering on its end to filter out irrelevant webhooks so we don't have to do any filtering on Sentry's end, this is to avoid keeping track of what's relevant to Codecov in Sentry. I'm adding a destination_type column to the WebhookPayload so we can distinguish between payloads meant for Codecov and other Sentry regions because when we make the request in the drain_mailbox we want to run different logic, using the CodecovApiClient. I also made the choice to use a different mailbox_name so that the forwarding doesn't interfere with the webhooks being sent to the region silos. Finally, because we want to forward ALL webhooks to Codecov, that means we're forwarding installation events, a webhook that in Sentry gets processed in the control silo, so that gets its own mailbox identifier "installation".
background
we are bridging Codecov functionality into the Sentry product surface. at least initially, we will do this by writing Sentry API endpoints that are effectively proxies to the Codecov API. this PR introduces the class that those Sentry endpoints will use to make their requests
the Codecov API's base url is configurable via the
codecov.base-urloption so that, when working locally, we can direct Sentry to talk to a local instance of Codecov (or a staging instance, or something else).i have not worked in Sentry's actual codebase before - please advise on code layout, best practices, anything else. thanks!
details
thin wrapper around
requeststhat adds anAuthorization: Bearer <JWT>header + uses a configurable base URL for requests to the Codecov API. the JWT indicates some context about the logged-in Sentry user that Codecov needs to fulfill the request.the JWT is signed (HS256) using a secret shared between Codecov and Sentry and accessed through the
codecov.api-bridge-signing-secretoption. there is actually already a shared secret for signing JWTs ingetsentry, and https://github.com/getsentry/getsentry/pull/17508 populates the new option with that existing secret.relies on https://github.com/getsentry/getsentry/pull/17508
related to codecov/umbrella#149
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.