feat(preprod): Set up snapshot get API#108199
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
github-actions
bot
added
the
Scope: Backend
| T = TypeVar("T") | ||
|
|
||
|
|
||
| def parse_request_with_pydantic(request: Request, model: type[T]) -> T: |
There was a problem hiding this comment.
Wanted to share this as I like the idea of using pydantic over the current schemas
|
|
||
|
|
||
| class SnapshotImageResponse(BaseModel): | ||
| id: str |
There was a problem hiding this comment.
I'll need to add objectstore key here
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
src/sentry/preprod/api/models/snapshots/project_preprod_snapshot_models.py
Outdated
Show resolved
Hide resolved
rbro112
changed the title
rbro112
force-pushed
the
ryan/wireup_snapshot_get_api
branch
from
f0bf87f to
af96c5b
Compare
| ) | ||
|
|
||
| offset = request_data.offset | ||
| limit = request_data.limit |
There was a problem hiding this comment.
should we use the standardized OffsetPaginator for pagination like we do in list build endpoints we already have? I remember @ryan953 pushing for it
There was a problem hiding this comment.
I still recommend it! It helps with all parts of pagination. It works well enough with our ui components to show prev/next buttons by returning the Link header in a common format that includes a cursor value, and you can also optionally return the X-Hits header which makes it possible to know how many items there are in total.
As-is I don't see this endpoint returning a cursor, so the ui would need to presumably adjust the offset itself to make requests for the 2nd and 3rd pages, and you'd have no idea how many pages there are in total.
| head_artifact_id: str | ||
| base_artifact_id: str | None = None # Only present for diffs | ||
| state: PreprodArtifact.ArtifactState | ||
| comparison_type: SnapshotComparisonType |
There was a problem hiding this comment.
im fine keeping this but curious if it's even necessary since if base_artifact_id is missing, doesn't that imply it's a diff?
rbro112
force-pushed
the
ryan/wireup_snapshot_get_api
branch
from
af96c5b to
43725f8
Compare
src/sentry/preprod/api/models/snapshots/project_preprod_snapshot_models.py
Show resolved
Hide resolved
…shot endpoint The snapshot GET endpoint was using parse_request_with_pydantic which parses request.body as JSON, causing 400 errors on GET requests. Switch to constructing SnapshotGetRequest from request.GET query params. Also add validation bounds on offset/limit, make ImageMetadata.display_name optional for backwards compat, and update test assertions for key field.
rbro112
force-pushed
the
ryan/wireup_snapshot_get_api
branch
from
3089e94 to
2961584
Compare
| display_name: str | None = None | ||
| file_name: str |
There was a problem hiding this comment.
Bug: The ImageMetadata model now requires the file_name field. Loading old snapshot manifests that lack this field will cause a validation error and fail.
Severity: CRITICAL
Suggested Fix
To maintain backwards compatibility, the file_name field in the ImageMetadata model should be made optional. For example, change file_name: str to file_name: str | None = None. This will allow the system to successfully parse old manifests that do not contain this field.
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/preprod/snapshots/manifest.py#L7-L8
Potential issue: The `ImageMetadata` Pydantic model has been updated to make the
`file_name` field mandatory. However, existing snapshot manifests stored in object
storage were created with an older schema and do not contain this field. When the GET
endpoint for snapshots attempts to load and parse one of these old manifests, Pydantic
will raise a `ValidationError` because the required `file_name` is missing. This
exception is caught and results in a 500 error, effectively making all pre-existing
snapshots inaccessible.
Wires up the snapshots GET api that we'll use for both snapshots and snapshot diffs. Frontend implemented in #108278
Wires up the snapshots GET api that we'll use for both snapshots and snapshot diffs. Frontend implemented in #108278
Wires up the snapshots GET api that we'll use for both snapshots and snapshot diffs. Frontend implemented in #108278
3 participants
Wires up the snapshots GET api that we'll use for both snapshots and snapshot diffs. Frontend implemented in #108278