Conversation
Backend Test FailuresFailures on
|
| all_artifacts, | ||
| snapshot_metrics_map, | ||
| ) | ||
| else: |
There was a problem hiding this comment.
https://github.com/EmergeTools/hackernews/runs/66045496060
can't we have valid solo success cases that aren't the first upload? like on main branch?
There was a problem hiding this comment.
Good call, adding the proper support here will require checking the presence of a base_sha but no matching build. Will implement this as a follow up - EME-921
For now, we'll return SUCCESS to match existing Emerge behavior
| # TODO(EME-921) Add logic to fail if there's any base_sha set but no base artifact | ||
| status = StatusCheckStatus.SUCCESS | ||
| title, subtitle, summary = format_missing_base_snapshot_status_check_messages( | ||
| all_artifacts, | ||
| snapshot_metrics_map, | ||
| ) | ||
| else: |
There was a problem hiding this comment.
Bug: The query for has_previous_snapshots incorrectly handles NULL values in commit_comparison_id, as .exclude() will not filter them out, leading to an incorrect is_first_upload determination.
Severity: MEDIUM
Suggested Fix
Modify the query to explicitly handle NULL values. Chain a second .exclude() call, such as .exclude(preprod_artifact__commit_comparison_id__isnull=True), to ensure records with a null commit_comparison_id are also filtered from the result set.
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/vcs/status_checks/snapshots/tasks.py#L172-L178
Potential issue: The query to determine if previous snapshots exist uses
`.exclude(preprod_artifact__commit_comparison_id=commit_comparison.id)` to filter out
the current commit. However, the `commit_comparison` foreign key on the
`PreprodArtifact` model is nullable. The `.exclude()` filter will not match records
where `commit_comparison_id` is `NULL`. This causes the query to incorrectly include
older snapshots that have a null `commit_comparison_id`, leading the system to wrongly
conclude that `is_first_upload` is `False`. As a result, users may see an incorrect
status message, such as "No base snapshots found," instead of the correct "This looks
like your first snapshot upload" message.
Did we get this right? 👍 / 👎 to inform future reviews.
| else: | ||
| # TODO(EME-921) Add logic to fail if there's any base_sha set but no base artifact | ||
| status = StatusCheckStatus.SUCCESS | ||
| title, subtitle, summary = format_missing_base_snapshot_status_check_messages( |
There was a problem hiding this comment.
not sure if you want this to the message for the success case but I'll assume that will be addressed as part of EME-921
There was a problem hiding this comment.
Yeah I'll take that on as part of 921 (added a note), but I think having the message still show missing base for now is fine.

Adds solo snapshot status checks. Three cases:
First upload (success - no base to be found)

Uploaded and no base provided (success - main branch is the use case here)

Uploaded and base provided but not found (error)
