Skip to content

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

Merged
merged 8 commits into from
Nov 29, 2023
Merged

Refine workspace API #19138

merged 8 commits into from
Nov 29, 2023

Conversation

csweichel
Copy link
Contributor

@csweichel csweichel commented Nov 24, 2023

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

  1. Workspace create should works like before
  • ✅ Create workspace from dashboard
  • ❌ From prebuild
  • ✅ From snapshot
  • ✅ Add multiple projects for repo, and create ws based on that repo
  1. ✅ workspace pin / rename / share should works
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 in envvar.proto.

Documentation

Preview status

gitpod:summary

Build Options

Build
  • /werft with-werft
    Run the build with werft instead of GHA
  • leeway-no-cache
  • /werft no-test
    Run Leeway with --dont-test
Publish
  • /werft publish-to-npm
  • /werft publish-to-jb-marketplace
Installer
  • analytics=segment
  • with-dedicated-emulation
  • workspace-feature-flags
    Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-gce-vm
    If enabled this will create the environment on GCE infra
  • /werft preemptible
    Saves cost. Untick this only if you're really sure you need a non-preemtible machine.
  • with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh. If enabled, with-preview and with-large-vm will be enabled.
  • with-monitoring

/hold

@mustard-mh
Copy link
Contributor

Migration is addressed here #19153

@mustard-mh mustard-mh force-pushed the cw/refine-workspace-api branch from 49433ce to 94ea82b Compare November 28, 2023 14:26
@mustard-mh mustard-mh marked this pull request as ready for review November 28, 2023 15:15
@mustard-mh
Copy link
Contributor

mustard-mh commented Nov 28, 2023

I have tested changed methods (see PR description), could you take a look @akosyakov 🙏 ? Or I just approve and merge it?

@akosyakov
Copy link
Member

@mustard-mh you tested both with FF on and off?

@akosyakov
Copy link
Member

@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;
Copy link
Member

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?

Copy link
Contributor

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

Copy link
Member

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

@mustard-mh mustard-mh force-pushed the cw/refine-workspace-api branch from 059127d to ec732e5 Compare November 29, 2023 10:25
@mustard-mh mustard-mh force-pushed the cw/refine-workspace-api branch from ec732e5 to bd69a05 Compare November 29, 2023 10:41
Copy link
Member

@AlexTugarev AlexTugarev left a 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
Copy link
Member

Choose a reason for hiding this comment

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

nit: docs are swapped

Copy link
Contributor

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}${
Copy link
Member

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.

Copy link
Contributor

@mustard-mh mustard-mh Nov 29, 2023

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:
Copy link
Member

Choose a reason for hiding this comment

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

are those TODOs relevant?

Copy link
Contributor

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

Copy link
Contributor

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)

Copy link
Member

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

Copy link
Member

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

👍🏻

@roboquat roboquat merged commit da0c590 into main Nov 29, 2023
@roboquat roboquat deleted the cw/refine-workspace-api branch November 29, 2023 14:15
mustard-mh added a commit that referenced this pull request Nov 30, 2023
roboquat pushed a commit that referenced this pull request Nov 30, 2023
mustard-mh added a commit that referenced this pull request Dec 1, 2023
roboquat pushed a commit that referenced this pull request Dec 4, 2023
* Revert "Revert "Refine workspace API (#19138)" (#19172)"

This reverts commit a6f4722.

* Update to workspace cases

* Add new cases and fix

* Add timeout

* fixup

* Remove requirement of editor

* Update cases

* Bump version

* Update dashboard
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants