-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Refine workspace API #19138
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
Refine workspace API #19138
Conversation
6370502 to
49433ce
Compare
|
Migration is addressed here #19153 |
49433ce to
94ea82b
Compare
|
I have tested changed methods (see PR description), could you take a look @akosyakov 🙏 ? Or I just approve and merge it? |
|
@mustard-mh you tested both with FF on and off? |
|
@mustard-mh Could you rebase please? |
|
|
||
| string name = 9; | ||
| // spec is the configuration of the workspace that's required for the to start the workspace | ||
| WorkspaceSpec spec = 4; |
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.
@mustard-mh do we have an issue already to support this path?
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.
We have lots of TODOs for CreateAndStartWorkspace method, I will create an issue with TODOs list later. then decide if we need make them some standalone issues when we're going to implement 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.
ok, concern here it should be a primary way when we announce API
059127d to
ec732e5
Compare
ec732e5 to
bd69a05
Compare
AlexTugarev
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.
Tested the default behavior:
- start/stop/restart/delete workspaces in different orgs
- pin/share/snapshot workspace
| // | ||
| // +optional defaults to the user's default region | ||
| string region = 8; | ||
| // owner is the ID of the Gitpod user to whom we'll bill this workspace and who we consider responsible for its content |
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: docs are swapped
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.
Updated, thx! 👍
| const filteredActiveWorkspaces = activeWorkspaces.filter( | ||
| (info) => | ||
| `${info.name}${info.id}${info.contextUrl}${info.status?.gitStatus?.cloneUrl}${info.status?.gitStatus?.branch}` | ||
| `${info.metadata!.name}${info.id}${info.metadata!.originalContextUrl}${ |
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.
Is it always guaranteed that metadata is present?
Reading metadata! a lot let me think if we can add an assertion when setting the info value.
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.
@AlexTugarev Yes, metadata is always in Workspace. We agreed to use ! in this situation here #19022 (comment)
cc @akosyakov
| value: new GitInitializer({ | ||
| remoteUri: arg.workspace.context.normalizedContextURL, | ||
| upstreamRemoteUri: arg.workspace.context.upstreamRemoteURI, | ||
| // TODO: |
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.
are those TODOs relevant?
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.
No, we added lots of things in message Workspace, and it's the shape for the future (in next-gen). Add TODOs because we can't do convert currently
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.
We will create issue for them #19138 (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.
for initialisers we will need to look at it again, we may rather use new shapes instead of changing them back and forth
AlexTugarev
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.
👍🏻
This reverts commit da0c590.
Description
This PR refines the workspace API aligning it with the next iteration of the server -> workspace runner interface.
It's the result of many hours of discussion between @svenefftinge, @akosyakov and @csweichel
How to Test
https://cw-refine-2e01273d5f.preview.gitpod-dev.com/workspaces
Summary generated by Copilot
🤖[deprecated] Generated by Copilot at 6370502
Refactor the public API for Gitpod by moving the
EnvironmentVariablemessage to a top-level definition inenvvar.proto.Documentation
Preview status
gitpod:summary
Build Options
Build
Run the build with werft instead of GHA
Run Leeway with
--dont-testPublish
Installer
Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
If enabled this will build
install/previewIf enabled this will create the environment on GCE infra
Saves cost. Untick this only if you're really sure you need a non-preemtible machine.
Valid options are
all,workspace,webapp,ide,jetbrains,vscode,ssh. If enabled,with-previewandwith-large-vmwill be enabled./hold