Skip to content

feat(ci): run self-hosted upgrade test#113388

Open
aldy505 wants to merge 7 commits into
getsentry:masterfrom
aldy505:feat/self-hosted-upgrade-test
Open

feat(ci): run self-hosted upgrade test#113388
aldy505 wants to merge 7 commits into
getsentry:masterfrom
aldy505:feat/self-hosted-upgrade-test

Conversation

@aldy505
Copy link
Copy Markdown
Collaborator

@aldy505 aldy505 commented Apr 19, 2026

To prevent InconsistentMigrationHistory in the future.

Corresponding self-hosted PR: getsentry/self-hosted#4288

@aldy505 aldy505 requested a review from a team as a code owner April 19, 2026 14:09
Comment thread .github/workflows/self-hosted.yml Outdated
Comment thread .github/workflows/self-hosted.yml Outdated
Comment thread .github/workflows/self-hosted.yml Outdated
Comment thread .github/workflows/self-hosted.yml Outdated
@aldy505 aldy505 added the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label Apr 23, 2026
Comment thread .github/workflows/self-hosted.yml Outdated
@Dav1dde Dav1dde added Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests and removed Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests labels Apr 27, 2026
Copy link
Copy Markdown
Member

@kenzoengineer kenzoengineer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall makes sense to me, would want some eyes from @hubertdeng123 though

just had one comment

Comment thread .github/workflows/self-hosted.yml Outdated
vroom
uptime_checker
image_url: |-
ghcr.io/getsentry/sentry:${{ github.sha }}-amd64
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we only build images if the PR author is an OWNER or MEMBER, does this mean this step fails when external contributors make a PR?

Copy link
Copy Markdown
Member

@hubertdeng123 hubertdeng123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have an idea how long this will take to run in CI? I'd suspect this will take quite a while, and we'd be running tests on migrations which doesn't seem necessary.

TBH I'm pretty certain we don't need to even bring up self-hosted here. Instead, we can just checkout sentry at the latest release tag and run migrations up to latest using the dev environment

@aldy505
Copy link
Copy Markdown
Collaborator Author

aldy505 commented Apr 28, 2026

Do we have an idea how long this will take to run in CI? I'd suspect this will take quite a while, and we'd be running tests on migrations which doesn't seem necessary.

I would assume 15-20 mins

TBH I'm pretty certain we don't need to even bring up self-hosted here. Instead, we can just checkout sentry at the latest release tag and run migrations up to latest using the dev environment

Sounds great to me. How do we do that?

@hubertdeng123
Copy link
Copy Markdown
Member

hubertdeng123 commented Apr 28, 2026

I'd imagine it'd look something like this at the beginning, we perform migrations up to the latest tag. Then, checkout nightly and perform migrations up to nightly

- name: Get latest release tag
    id: release
    run: |
      TAG=$(gh api repos/getsentry/sentry/releases/latest --jq '.tag_name')
      echo "tag=$TAG" >> "$GITHUB_OUTPUT"

- uses: actions/checkout@sha
    with:
      ref: ${{ steps.release.outputs.tag }}
 
- name: Setup sentry env
     uses: ./.github/actions/setup-sentry
       with:
         mode: migrations

- name: Apply migrations
    run: |
      sentry upgrade --noinput

@aldy505 aldy505 mentioned this pull request Apr 30, 2026
18 tasks
@aldy505 aldy505 requested a review from hubertdeng123 April 30, 2026 14:28
@github-actions github-actions Bot removed the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label Apr 30, 2026
@aldy505 aldy505 added the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label Apr 30, 2026
runs-on: ubuntu-24.04
timeout-minutes: 30
needs: did-migration-change
if: ${{ needs.did-migration-change.outputs.modified == 'true' || needs.did-migration-change.outputs.added == 'true' }}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The upgrade-test CI job will not run for modified migration files because the migrations_modified filter is not defined in .github/file-filters.yml.
Severity: MEDIUM

Suggested Fix

Define the migrations_modified filter in .github/file-filters.yml using the modified: event qualifier, similar to how migrations_added is defined. This will ensure the dorny/paths-filter action correctly detects modified migration files and populates the migrations_modified output, allowing the upgrade-test job to trigger as intended.

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: .github/workflows/migrations.yml#L84

Potential issue: The `upgrade-test` job's conditional logic is intended to trigger for
both added and modified migration files. However, the check for modified files,
`needs.did-migration-change.outputs.modified == 'true'`, will never evaluate to true.
This is because the underlying output `steps.changes.outputs.migrations_modified` from
the `dorny/paths-filter` action is always empty, as the `migrations_modified` filter key
is not defined in `.github/file-filters.yml`. Consequently, the `upgrade-test` job is
silently skipped for all modified migration files, potentially allowing breaking changes
to be merged without testing.

Comment thread .github/workflows/migrations.yml
runs-on: ubuntu-24.04
timeout-minutes: 30
needs: did-migration-change
if: ${{ needs.did-migration-change.outputs.modified == 'true' || needs.did-migration-change.outputs.added == 'true' }}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upgrade test never triggers for modified migrations

Medium Severity

The modified == 'true' part of this condition will never be true because the migrations_modified filter is not defined in .github/file-filters.yml — only migrations_added exists. The needs.did-migration-change.outputs.modified value will always be an empty string, meaning this upgrade test only runs for newly added migrations, not for modified ones. Since the stated goal is to prevent InconsistentMigrationHistory, and modifying an existing migration's dependencies is one way to cause that error, this is a coverage gap.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d12be93. Configure here.

@github-actions github-actions Bot removed the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label May 3, 2026
Comment on lines +107 to +108
run: |
sentry upgrade --noinput
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The upgrade-test job may fail with a ModuleNotFoundError because it runs new migrations against a stale Python environment that lacks new dependencies from the PR.
Severity: MEDIUM

Suggested Fix

After checking out the PR's code with clean: false, re-synchronize the Python virtual environment using the PR's uv.lock file. Add a step to run uv sync --frozen --active to install any new dependencies before the sentry upgrade command is executed.

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: .github/workflows/migrations.yml#L107-L108

Potential issue: In the `upgrade-test` job, the Python virtual environment is created
based on an old release tag's dependencies. The workflow then checks out the pull
request's code but preserves the old, stale environment. When `sentry upgrade --noinput`
is executed, it runs new migration files from the PR. If a new migration imports a
module from a newly added external Python dependency, the command will fail with a
`ModuleNotFoundError` because the required dependency is not present in the virtual
environment.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hubertdeng123 is this true?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is true since you're checking out the new branch but not installing new dependencies

@aldy505 aldy505 added the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label May 5, 2026
@hubertdeng123
Copy link
Copy Markdown
Member

Do you have a successful run of this in CI? I'd like to see one to verify that this is working as intended

@github-actions github-actions Bot removed the Trigger: getsentry tests Once code is reviewed: apply label to PR to trigger getsentry tests label May 15, 2026
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit afac2a7. Configure here.

clean: false
- name: Apply migrations again to test upgrade path
run: |
sentry upgrade --noinput
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing dependency reinstall before second migration run

High Severity

After checking out the current ref (with clean: false), the upgrade-test job runs sentry upgrade --noinput without reinstalling Python dependencies. The setup-sentry action (which runs uv sync --frozen --active and fast_editable) is only executed against the release tag's uv.lock. If the current branch introduces any new dependencies, Django will fail to load apps/models with an ImportError, producing a false CI failure unrelated to migration consistency. Unlike the existing sql job which only runs git diff after checkout, this job needs the full sentry environment functional for the second sentry upgrade call.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit afac2a7. Configure here.

@aldy505
Copy link
Copy Markdown
Collaborator Author

aldy505 commented May 17, 2026

Do you have a successful run of this in CI? I'd like to see one to verify that this is working as intended

@hubertdeng123 Nope, do you have any idea on how to test this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants