Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update migrations list command to show migrations that no longer exist in the codebase #6299

Merged
merged 4 commits into from
Sep 13, 2024

Conversation

davidtsuk
Copy link
Contributor

@davidtsuk davidtsuk commented Sep 12, 2024

Fixes #2159

Running snuba migrations list will now show migrations that are in clickhouse but no longer in the codebase (e.g. because snuba was downgraded).

Testing

Deleted migration in codebase to check that it shows up in the list.
image

Copy link

codecov bot commented Sep 12, 2024

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
1 1 0 0
View the top 1 failed tests by shortest run time
 tests.admin.clickhouse_migrations.test_migration_checks
Stack Traces | 0s run time
No failure message available

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

Copy link
Member

@onkar onkar left a comment

Choose a reason for hiding this comment

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

Can you add unit test covering the case of completed but non-existent migrations?

Also, is such a migration configuration possible? MigrationDetails("0001", Status.COMPLETED, True, False) where migration status is completed and it's non-existent but still blocked?

Comment on lines 153 to 154
for group, migration_id in migration_status.keys():
clickhouse_group_migrations.setdefault(group, []).append(migration_id)
Copy link
Member

Choose a reason for hiding this comment

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

nit: Would defaultdict() be a better choice here?

clickhouse_group_migrations = defaultdict(set)
for group, migration_id in migration_status.keys():
    clickhouse_group_migrations[group].add(migration_id)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is probably better yeah

snuba/migrations/runner.py Show resolved Hide resolved
@davidtsuk
Copy link
Contributor Author

davidtsuk commented Sep 13, 2024

Also, is such a migration configuration possible? MigrationDetails("0001", Status.COMPLETED, True, False) where migration status is completed and it's non-existent but still blocked?

I'm not sure if this is possible (maybe if a migration file is deleted while the migration is still being processed).

@davidtsuk davidtsuk marked this pull request as ready for review September 13, 2024 18:22
@davidtsuk davidtsuk requested a review from a team as a code owner September 13, 2024 18:22
Copy link
Member

@onkar onkar left a comment

Choose a reason for hiding this comment

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

LGTM 👍 thanks for addressing the comments.

@davidtsuk davidtsuk merged commit 68b4ebd into master Sep 13, 2024
30 checks passed
@davidtsuk davidtsuk deleted the david/fix/migrations-list branch September 13, 2024 18:52
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.

migrations list does not show migrations not in list
2 participants