-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[ws-manager-mk2] Implement MarkActive #16181
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
|
/werft run with-preview with-gce-vm with-wsman-mk2 with-large-vm 👍 started the job as gitpod-build-wv-mk2-mark-active.1 |
|
/werft run with-integration-tests=workspace with-large-vm=true 👍 started the job as gitpod-build-wv-mk2-mark-active.2 |
kylos101
left a comment
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.
Nice work, @WVerlaek, super clean to read and thorough. Just a couple thoughts, nothing blocking.
/hold
Feel free to remove the hold, either after handling the suggestion and nit, or not.
| }) | ||
| } | ||
| if err != nil { | ||
| log.WithError(err).WithFields(log.OWI("", "", workspaceID)).Warn("was unable to mark workspace properly") |
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.
Suggestion, non-blocking:
Include req.Closed and isMarkedClosed in the warning message, so that we know which path and error were encountered.
| Type: string(workspacev1.WorkspaceConditionFirstUserActivity), | ||
| Status: metav1.ConditionTrue, | ||
| LastTransitionTime: metav1.NewTime(now), | ||
| Reason: "FirstActivity", |
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.
Nit, non-blocking:
Why not a reason of FirstUserActivity?
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.
good catch - i've changed the Reason to MarkActiveRequest in line with the Closed conditions above
kylos101
left a comment
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.
👍
|
/werft run with-preview with-gce-vm with-wsman-mk2 with-large-vm 👍 started the job as gitpod-build-wv-mk2-mark-active.3 |
kylos101
left a comment
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.
👍 had to rebuild so that I could test with mk2. So great to kubectl get workspaces. 😃
463ee02 to
3863ce7
Compare
|
/unhold |
Description
Splitting out from #16122
This PR implements
MarkActivein ws-manager-mk2.Also contains some drive-by improvements (vscode workspace, remove error log in reconciler if resource is not found, this is normal and happens on deletion)
Related Issue(s)
Part of #11416
How to test
Open in preview env, check that the
FirstUserActivitycondition gets set on the workspace CR when opening a workspace.Release Notes
Documentation
Build Options:
Experimental feature to run the build with GitHub Actions (and not in Werft).
leeway-target=components:all
Run Leeway with
--dont-testPreview Environment Options:
If enabled this will build
install/previewIf enabled this will create the environment on GCE infra
Valid options are
all,workspace,webapp,ide,jetbrains,vscode,ssh