[public-api] aligns with latest guidelines #19038

akosyakov · 2023-11-08T12:11:23Z

Description

This PR aligns API definition and implementation with our latest guidelines.

Summary generated by Copilot

`🤖 Generated by Copilot at 27a002d`

This pull request updates the protobuf files and the generated code for the public API of Gitpod, to use FieldMask for partial updates, to remove optional keywords from proto3 fields, and to fix some inconsistencies and errors in the types and default values.

Related Issue(s)

Fixes #

How to test

Test that you can update org name, workspace sharing, default image, and membership without issues.

Documentation

Preview status

Gitpod was successfully deployed to your preview environment.

🏷️ Name - ak-only-opac2d8f7a3d
🔗 URL - ak-only-opac2d8f7a3d.preview.gitpod-dev.com/workspaces.
📚 Documentation - See our internal documentation for information on how to interact with your preview environment.
📦 Version - ak-only_optional_in_update-gha.19785
🗒️ Logs - GCP Logs Explorer

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
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

components/server/src/api/organization-service-api.ts

akosyakov · 2023-11-13T08:59:57Z

/gh run recreate-vm=true

Comment triggered a workflow run

Started workflow run: 6847733871

recreate_vm: true

akosyakov · 2023-11-13T10:13:43Z

components/public-api/gitpod/v1/organization.proto

-  optional string avatar_url = 4;
-  optional string full_name = 5;
-  optional string email = 6;
+  string avatar_url = 4;


in this file I drop optional keywords from everywhere but to make update calls partial

akosyakov · 2023-11-13T10:14:00Z

components/public-api/gitpod/v1/workspace.proto

@@ -19,15 +19,15 @@ service WorkspaceService {
  rpc WatchWorkspaceStatus(WatchWorkspaceStatusRequest) returns (stream WatchWorkspaceStatusResponse) {}
 }

-message GetWorkspaceRequest { string id = 1; }
+message GetWorkspaceRequest { string workspace_id = 1; }


fixing field name conventions + drop optional keywords from everywhere

akosyakov · 2023-11-13T10:14:38Z

components/gitpod-db/src/typeorm/team-db-impl.ts

@@ -366,28 +366,19 @@ export class TeamDBImpl extends TransactionalDBImpl<TeamDB> implements TeamDB {
        });
    }

-    public async setOrgSettings(orgId: string, settings: Partial<OrganizationSettings>): Promise<void> {
+    public async setOrgSettings(orgId: string, settings: Partial<OrganizationSettings>): Promise<OrganizationSettings> {


moving logic into service from DB, making sure that update returns updated data

akosyakov · 2023-11-13T10:15:15Z

components/gitpod-protocol/src/messaging/error.ts

@@ -37,41 +36,18 @@ export namespace ApplicationError {
            throw e;
        }
    }
-
-    export function fromGRPCError(e: any, data?: any): ApplicationError {


we should never mindlessly pump error like that, we should do proper translation and if we cannot translate use INTERNAL to iterate on it

akosyakov · 2023-11-13T10:15:46Z

components/gitpod-protocol/src/messaging/error.ts

@@ -24,7 +23,7 @@ export class ApplicationError extends Error {

 export namespace ApplicationError {
    export function hasErrorCode(e: any): e is Error & { code: ErrorCode; data?: any } {
-        return e && e.code !== undefined;
+        return ErrorCode.is(e["code"]);


not all errors with code are ApplicationError, we will consider in the past some typeorm and Node.js errors as AppilcationErorrs, while they should be Internal

akosyakov · 2023-11-13T10:16:10Z

components/public-api/buf.yaml

@@ -5,6 +5,8 @@ breaking:
  ignore:
    # Do not enforce breaking change detection for the experimental package
    - gitpod/experimental
+    # TODO enable again after landing style changes
+    - gitpod/v1


temporary since we are still using JSON-RPC shim

akosyakov · 2023-11-13T10:16:48Z

components/server/src/orgs/default-workspace-image-validator.ts

+ * See License.AGPL.txt in the project root for license information.
+ */
+
+export type DefaultWorkspaceImageValidator = (userId: string, imageRef: string) => Promise<void>;


I moved defaulta wokrspace image validation to organizaion-service, but then it depends on workspace-service. So decouple it we use an intermediate service with lazy loading.

akosyakov · 2023-11-13T10:17:20Z

components/server/src/workspace/gitpod-server-impl.ts

@@ -2358,9 +2358,6 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable {
        const user = await this.checkAndBlockUser("updateOrgSettings");
        traceAPIParams(ctx, { orgId, userId: user.id });
        await this.guardTeamOperation(orgId, "update");
-        if (settings.defaultWorkspaceImage?.trim()) {


we forgot to move while doing org migration

akosyakov · 2023-11-13T10:18:46Z

components/dashboard/src/teams/TeamSettings.tsx

@@ -282,7 +282,7 @@ function WorkspaceImageButton(props: {
        };
    }

-    const image = props.settings?.defaultWorkspaceImage ?? props.defaultWorkspaceImage ?? "";
+    const image = props.settings?.defaultWorkspaceImage || props.defaultWorkspaceImage || "";


This one is potentially breaking for old dashboard against new server, but affect is not significant and resolved by the page reload.

Ideally we need to deliver Huiwen's PR to stop worrying about it.

We discussed that a risk is pretty low, given that update is very rare operation and we deploy to cells in non working hours. Anyway we wait today for #19042 and if it takes longer then we land this one. cc @mustard-mh

@mustard-mh I thinkg there is not problem actually, JSON-RPC is backward compatible, i.e. returns undefined value and old dashboard is going to use old gRPC types with optional, so it should not break, does it make sense?

mustard-mh · 2023-11-13T17:19:09Z

components/server/src/api/organization-service-api.ts

+        if (!uuidValidate(req.organizationId)) {
+            throw new ConnectError("organizationId is required", Code.InvalidArgument);
+        }
+        if (typeof req.name !== "string") {


We don't allow empty name?

mustard-mh · 2023-11-13T17:34:06Z

components/server/src/workspace/workspace-service.ts

+                let message = details;
+                // strip confusing prefix
+                if (details.startsWith("cannt resolve base image ref: ")) {
+                    message = details.substring("cannt resolve base image ref: ".length);


Respond code with be something else, see test case which use it

nit-fixing since it doesn't break UX

I'm not sure what I should change?

~~Based on the code you added, you want to remove the prefix cannot resolve image but it will not match since the respond rpc code is not grpc.status.INVALID_ARGUMENT~~

Nope, I read it wrong. Sorry for confusing

I think it is covered because it is || not &&, but I agree though that explicit check for NOT_FOUND and UNAUTHORIZED makes sense here too.

We need generally align how we propagate errors thorugh the system and we don't break contracts.

mustard-mh · 2023-11-13T17:36:50Z

Test with preview env, LGTM

/hold in case you want to fix comments above or wait #19042 to be deployed

akosyakov · 2023-11-14T11:08:49Z

/unhold

roboquat added do-not-merge/hold do-not-merge/work-in-progress size/M labels Nov 8, 2023

akosyakov force-pushed the ak/only_optional_in_update branch 4 times, most recently from 37e26af to 27c1691 Compare November 8, 2023 21:41

roboquat added size/L and removed size/M labels Nov 8, 2023

akosyakov force-pushed the ak/only_optional_in_update branch 3 times, most recently from a8bd622 to 80888c3 Compare November 9, 2023 20:28

roboquat added size/XL and removed size/L labels Nov 9, 2023

akosyakov force-pushed the ak/only_optional_in_update branch 4 times, most recently from f3e41f8 to da87c39 Compare November 10, 2023 09:32

roboquat added size/XXL and removed size/XL labels Nov 10, 2023

akosyakov force-pushed the ak/only_optional_in_update branch from da87c39 to ce5e2de Compare November 10, 2023 09:32

akosyakov changed the title ~~[public-api] only allow optional in update~~ [public-api] aligns with latest guidelines Nov 10, 2023

akosyakov force-pushed the ak/only_optional_in_update branch 3 times, most recently from ffb60d1 to d1b69a9 Compare November 10, 2023 09:52

roboquat added size/XL and removed size/XXL labels Nov 10, 2023

akosyakov force-pushed the ak/only_optional_in_update branch from d1b69a9 to a68f17f Compare November 10, 2023 11:19

roboquat added size/XXL and removed size/XL labels Nov 10, 2023

roboquat added the size/XL label Nov 10, 2023

akosyakov force-pushed the ak/only_optional_in_update branch from 95065d2 to 6af5924 Compare November 10, 2023 17:15

filiptronicek reviewed Nov 12, 2023

View reviewed changes

components/server/src/api/organization-service-api.ts Show resolved Hide resolved

akosyakov marked this pull request as ready for review November 13, 2023 10:12

akosyakov requested a review from a team as a code owner November 13, 2023 10:12

roboquat removed the do-not-merge/work-in-progress label Nov 13, 2023

github-actions bot added the team: team-experience label Nov 13, 2023

akosyakov commented Nov 13, 2023

View reviewed changes

akosyakov requested review from AlexTugarev and mustard-mh November 13, 2023 10:19

[public-api] align with API guidelines

9d28cf8

akosyakov force-pushed the ak/only_optional_in_update branch from 6af5924 to 9d28cf8 Compare November 13, 2023 11:36

mustard-mh reviewed Nov 13, 2023

View reviewed changes

mustard-mh approved these changes Nov 13, 2023

View reviewed changes

roboquat removed the do-not-merge/hold label Nov 14, 2023

roboquat merged commit 0ec0037 into main Nov 14, 2023
16 checks passed

roboquat deleted the ak/only_optional_in_update branch November 14, 2023 11:09

akosyakov mentioned this pull request Nov 14, 2023

[papi] migrate ListWorkspaces and WatchWorkspaceStatus #19022

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[public-api] aligns with latest guidelines #19038

[public-api] aligns with latest guidelines #19038

akosyakov commented Nov 8, 2023 •

edited

Loading

akosyakov commented Nov 13, 2023 •

edited by github-actions bot

Loading

akosyakov Nov 13, 2023

akosyakov Nov 13, 2023

akosyakov Nov 13, 2023

akosyakov Nov 13, 2023

akosyakov Nov 13, 2023

akosyakov Nov 13, 2023

akosyakov Nov 13, 2023

akosyakov Nov 13, 2023

akosyakov Nov 13, 2023

akosyakov Nov 13, 2023

akosyakov Nov 14, 2023

mustard-mh Nov 13, 2023

mustard-mh Nov 13, 2023 •

edited

Loading

akosyakov Nov 14, 2023

mustard-mh Nov 14, 2023 •

edited

Loading

akosyakov Nov 14, 2023

mustard-mh commented Nov 13, 2023

akosyakov commented Nov 14, 2023

[public-api] aligns with latest guidelines #19038

[public-api] aligns with latest guidelines #19038

Conversation

akosyakov commented Nov 8, 2023 • edited Loading

Description

🤖 Generated by Copilot at 27a002d

Related Issue(s)

How to test

Documentation

Preview status

Build Options

akosyakov commented Nov 13, 2023 • edited by github-actions bot Loading

Comment triggered a workflow run

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustard-mh Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustard-mh Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustard-mh commented Nov 13, 2023

akosyakov commented Nov 14, 2023

akosyakov commented Nov 8, 2023 •

edited

Loading

`🤖 Generated by Copilot at 27a002d`

akosyakov commented Nov 13, 2023 •

edited by github-actions bot

Loading

mustard-mh Nov 13, 2023 •

edited

Loading

mustard-mh Nov 14, 2023 •

edited

Loading