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

[db] Add statusVersion to prebuilds to track status version #9114

Merged
merged 6 commits into from
Apr 5, 2022

Conversation

easyCZ
Copy link
Member

@easyCZ easyCZ commented Apr 4, 2022

Description

Related Issue(s)

How to test

  1. Preview works
  2. New prebuilds can start
  3. Column statusVersion appears in the DB

Release Notes

NONE

Documentation

/hold
/uncc

@roboquat roboquat added size/S and removed size/XS labels Apr 4, 2022
@roboquat roboquat added size/M and removed size/S labels Apr 4, 2022
Sunset17
Sunset17 previously approved these changes Apr 4, 2022
@easyCZ easyCZ changed the title [db] Add stateVersion to prebuilds to track status version [db] Add statusVersion to prebuilds to track status version Apr 4, 2022
@@ -634,6 +634,7 @@ export interface PrebuiltWorkspace {
buildWorkspaceId: string;
creationTime: string;
state: PrebuiltWorkspaceState;
statusVersion: number;
Copy link
Member Author

Choose a reason for hiding this comment

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

Not entirely happy that the statusVersion needs to be exposed in the protocol and sent to clients. Is there a way (without significantly changing the system) to have this data only available to internal components and continue not having it in clients?

Copy link
Member

Choose a reason for hiding this comment

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

continue not having it in clients?

You could :

  1. introduce a sub-shape of that we use in protocol, and a more expanded shape we expose in GitpodService
  2. and hand-remove this field when sending it to clients (we do this with some sensitive field of WorkspaceInstance in GitpodServerImpl)
  3. but I'm not sure it warrant's the effort, now and for this single field. Let's hope make it better with the new API 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, that's what I was thinking as well. Unless there is a strong objection, I'll leave it as is for now.

@easyCZ easyCZ marked this pull request as ready for review April 5, 2022 07:24
@easyCZ easyCZ requested a review from a team April 5, 2022 07:24
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Apr 5, 2022
@geropl
Copy link
Member

geropl commented Apr 5, 2022

@easyCZ I wonder if we could place the field into WorkspaceInstance generically, because there we'll have to have it as well. And then we have duplication. 🤔

@easyCZ
Copy link
Member Author

easyCZ commented Apr 5, 2022

@easyCZ I wonder if we could place the field into WorkspaceInstance generically, because there we'll have to have it as well. And then we have duplication. 🤔

I'd rather avoid inheritance for things like the DB schema and core protocol. It introduces coupling which can create unintended side-effects (especially on the DB level) when you try to break it up.

I'm happy to compose it into both, however, if you think it's needed.

@geropl
Copy link
Member

geropl commented Apr 5, 2022

I'd rather avoid inheritance for things like the DB schema and core protocol.

Hm, I'd rather stick to the current idea of "data deduplication" (inheritance 👻 ) even if maintain the tight coupling between both tables. If we want to split it out we have to look at it more holistically I fear.

There might be a reason for duplication, though, especially for prebuilds. But I cannot think of one ad-hoc. 🤔

@easyCZ
Copy link
Member Author

easyCZ commented Apr 5, 2022

From what I can see, the ORM definition of Prebuilds does not currently have any inheritance specified [file].

And neither does the protocol [file]. Where exactly do you want to add inheritance?

I'd be open to defining an interface which has a field statusVersion which we can use to type the objects but YAGNI?

@geropl
Copy link
Member

geropl commented Apr 5, 2022

Summary: We're going with the statusVersion field in the prebuilds tabe
Reasoning:
we want to have statusVersion the API-facing shape
we agree that WorkspaceInstance will use the same field at some point
it's unpractical to read statusVersion from prebuiold (2-join Workspace->WorkspaceInstance.statusVersion)
we're taking the slight risk of data-duplication/-races in favor of
following the current pattern for API shapes (same as internal)
following the current pattern by having all fields in the shape in the DB table
Also, statusVersion tells us something about the data quality of all fields in the table (data provenance), so might make sense to duplicate it in the first place 🙃

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

LGTM

@easyCZ
Copy link
Member Author

easyCZ commented Apr 5, 2022

Thanks for the review and discussion @geropl!

/unhold

@roboquat roboquat merged commit e59efde into main Apr 5, 2022
@roboquat roboquat deleted the mp/prebuild-state-version-db branch April 5, 2022 08:22
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Apr 7, 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.

4 participants