Skip to content

[db] Soft delete entries in the db_volume_snapshot table #13391

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

Merged
merged 5 commits into from
Sep 30, 2022

Conversation

andrew-farries
Copy link
Contributor

@andrew-farries andrew-farries commented Sep 28, 2022

Description

Replace the existing hard-deletion of rows in the db_volume_snapshot table with soft-deletion.

This will allow db-sync to propagate deletions between clusters.

Related Issue(s)

Follow up to #13348

Part of #9198

How to test

Unit tests

Release Notes

NONE

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

@andrew-farries andrew-farries requested a review from a team September 28, 2022 07:43
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Sep 28, 2022
@andrew-farries
Copy link
Contributor Author

/hold

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-af-soft-delete-in-volume-snapshot-table.6 because the annotations in the pull request description changed
(with .werft/ from main)

@andrew-farries andrew-farries force-pushed the af/soft-delete-in-volume-snapshot-table branch from 9033463 to ee95f69 Compare September 29, 2022 06:16
@roboquat roboquat added size/M and removed size/XS labels Sep 29, 2022
Andrew Farries added 4 commits September 29, 2022 07:21
Replace the existing hard-deletion of volume snapshot rows in the
database with soft-deletion that will allow `db-sync` to propagate
deletions between clusters.
For each workspaceId, add a deleted volume snapshot.
Filter out deleted volume snapshots from the result sets.
Add tests (`testCantFindDeletedVolumeSnapshotById` fails).

The failing test is due to `findVolumeSnapshotById` not taking deleted
entries into account.
Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

LGTM, tho not an expert on our TypeORM models

/hold for Q

return volumeSnapshots
.createQueryBuilder("vs")
.where("vs.deleted = 0")
.andWhereInIds(volumeSnapshotId)
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between findOne and andWhereInIds?

Copy link
Member

Choose a reason for hiding this comment

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

Wondering the same. Using .andWhere("vs.id = :id", { id: volumneSnapshotId }) would be more direct, and require less TypeORM knowledge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've rebased to make this change.

Filter soft deleted entries out of the result set.
@andrew-farries andrew-farries force-pushed the af/soft-delete-in-volume-snapshot-table branch from f598cc1 to 3f1dc66 Compare September 29, 2022 09:08
@roboquat roboquat merged commit aa3c8ac into main Sep 30, 2022
@roboquat roboquat deleted the af/soft-delete-in-volume-snapshot-table branch September 30, 2022 06:39
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/M team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants