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

[gitpod-db] add new DB entity for VolumeSnapshot #9810

Merged
merged 1 commit into from
May 9, 2022
Merged

Conversation

sagor999
Copy link
Contributor

@sagor999 sagor999 commented May 5, 2022

Description

This adds a new DB entity that is used by #9475

Related Issue(s)

Part of #9475

How to test

This just adds a new entity and shouldn't affect anything else

Release Notes

none

Documentation

@sagor999 sagor999 requested a review from a team May 5, 2022 22:46
@github-actions github-actions bot added team: webapp Issue belongs to the WebApp team and removed size/L labels May 5, 2022
);
}

public async down(queryRunner: QueryRunner): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

Do you really want to drop the table in case we roll-back? I think here it's sensible to not drop it and keep it around, just unused. It's risky in the sense that server is never just a single replica, so we can't guarantee we won't be running code which will try to access the table and another which won't. Normally, we'd first create the table without any usage and then in later PRs, we'd start using it, such that it doesn't get dropped if the later PR needs to be reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I was following other migration examples, and thought that was the way to do it. 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@easyCZ updated. PTAL 🙏

@easyCZ easyCZ self-assigned this May 6, 2022
@sagor999 sagor999 force-pushed the pavel/snapshotdb branch from 8517306 to e85ea71 Compare May 9, 2022 00:45
@roboquat roboquat added the size/L label May 9, 2022
@sagor999 sagor999 force-pushed the pavel/snapshotdb branch from e85ea71 to 8674585 Compare May 9, 2022 01:17
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. I'd remove the commented out parts as they are not really useful directly.

Adding hold for you to merge at your will.
/hold

@sagor999 sagor999 force-pushed the pavel/snapshotdb branch from 8674585 to c1d011b Compare May 9, 2022 14:28
@sagor999
Copy link
Contributor Author

sagor999 commented May 9, 2022

/unhold

@roboquat roboquat merged commit f1c142b into main May 9, 2022
@roboquat roboquat deleted the pavel/snapshotdb branch May 9, 2022 14:38
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels May 13, 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/L team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants