-
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
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! 👍
@@ -47,14 +47,18 @@ const WorkspacesPage: FunctionComponent = () => { | |||
const { filteredActiveWorkspaces, filteredInactiveWorkspaces } = useMemo(() => { | |||
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
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
EnvironmentVariable
message 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-test
Publish
Installer
Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
If enabled this will build
install/preview
If 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-preview
andwith-large-vm
will be enabled./hold