-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[bridge] Extracting updating of prebuilds into PrebuildUpdater (2/3) #10425
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/hold because of merge conflicts after 1/3 got merged
ddd06f0
to
40a3e5d
Compare
/unhold |
@@ -462,24 +466,11 @@ export class WorkspaceManagerBridge implements Disposable { | |||
instance.stoppedTime = new Date().toISOString(); | |||
promises.push(this.workspaceDB.trace({}).storeInstance(instance)); | |||
promises.push(this.onInstanceStopped({}, ri.workspace.ownerId, instance)); | |||
promises.push(this.stopPrebuildInstance(ctx, instance)); | |||
promises.push(this.prebuildUpdater.stopPrebuildInstance(ctx, instance)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@geropl I don't think this does anything useful. Looks to me like it would update the workspace instance that got set to 'stopped' a few lines above now to 'aborted'. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to me like it would update the workspace instance that got set to 'stopped' a few lines above now to 'aborted'. Am I missing something?
It updates the PrebuiltWorkspace
that's corresponding that WorkspaceInstance
to aborted
, which would otherwise not bet updated.
In what context are you looking into this? Can I help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It updates the PrebuiltWorkspace that's corresponding that WorkspaceInstance to aborted, which would otherwise not bet updated.
Why does it do that? And how can a prebuild not be finished when there is a workspace based on it?
In what context are you looking into this? Can I help?
Just reading the code and trying to make sense of it 😇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, got it now. Forget it 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What confused me is that the method is called stopPrebuildInstance
while it only aborts the PrebuildWorkspace in the database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good 😉
Description
This PR extract all logic related to updating prebuilds from src/ee/bridge behind a new interface
PrebuildUpdater
with two implementations: Noop (non-ee version) and DB (ee version) (2/3 in the chain)Related Issue(s)
Context: #9395
How to test
kubectl logs ws-manager-bridge- ws-manager-bridge
Release Notes
Documentation