Skip to content
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.

Sign up for GitHub

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

[public-api] aligns with latest guidelines #19038

Merged
merged 1 commit into from
Nov 14, 2023
Merged

[public-api] aligns with latest guidelines #19038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions components/dashboard/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"version": "0.0.0",
"private": true,
"dependencies": {
"@bufbuild/protobuf": "^1.3.3",
"@connectrpc/connect": "1.1.2",
"@connectrpc/connect-web": "1.1.2",
"@gitpod/gitpod-protocol": "0.1.5",
Expand Down
6 changes: 3 additions & 3 deletions components/dashboard/src/components/PrebuildLogs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,11 @@ export default function PrebuildLogs(props: PrebuildLogsProps) {
// Try get hold of a recent WorkspaceInfo
try {
const request = new GetWorkspaceRequest();
request.id = props.workspaceId;
request.workspaceId = props.workspaceId;
const response = await workspaceClient.getWorkspace(request);
setWorkspace({
instanceId: response.item?.status?.instanceId,
phase: response.item?.status?.phase?.name,
instanceId: response.workspace?.status?.instanceId,
phase: response.workspace?.status?.phase?.name,
});
} catch (err) {
console.error(err);
Expand Down
10 changes: 7 additions & 3 deletions components/dashboard/src/data/organizations/update-org-mutation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@
* See License.AGPL.txt in the project root for license information.
*/

import { Organization } from "@gitpod/gitpod-protocol";
import { useMutation } from "@tanstack/react-query";
import { getGitpodService } from "../../service/service";
import { useCurrentOrg, useOrganizationsInvalidator } from "./orgs-query";
import { organizationClient } from "../../service/public-api";
import { Organization } from "@gitpod/public-api/lib/gitpod/v1/organization_pb";

type UpdateOrgArgs = Pick<Organization, "name">;

Expand All @@ -21,7 +21,11 @@ export const useUpdateOrgMutation = () => {
throw new Error("No current organization selected");
}

return await getGitpodService().server.updateTeam(org.id, { name });
const response = await organizationClient.updateOrganization({
organizationId: org.id,
name,
});
return response.organization!;
},
onSuccess(updatedOrg) {
// TODO: Update query cache with new org prior to invalidation so it's reflected immediately
Expand Down
8 changes: 3 additions & 5 deletions components/dashboard/src/data/organizations/update-org-settings-mutation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,10 @@ export const useUpdateOrgSettingsMutation = () => {
mutationFn: async ({ workspaceSharingDisabled, defaultWorkspaceImage }) => {
const settings = await organizationClient.updateOrganizationSettings({
organizationId: teamId,
settings: {
workspaceSharingDisabled: workspaceSharingDisabled || false,
defaultWorkspaceImage,
},
workspaceSharingDisabled: workspaceSharingDisabled || false,
defaultWorkspaceImage,
});
return settings.settings || new OrganizationSettings();
return settings.settings!;
},
onSuccess: invalidator,
});
Expand Down
2 changes: 1 addition & 1 deletion components/dashboard/src/data/setup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import * as ConfigurationClasses from "@gitpod/public-api/lib/gitpod/v1/configur
// This is used to version the cache
// If data we cache changes in a non-backwards compatible way, increment this version
// That will bust any previous cache versions a client may have stored
const CACHE_VERSION = "2";
const CACHE_VERSION = "3";

export function noPersistence(queryKey: QueryKey): QueryKey {
return [...queryKey, "no-persistence"];
Expand Down
22 changes: 11 additions & 11 deletions components/dashboard/src/service/json-rpc-organization-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export class JsonRpcOrganizationClient implements PromiseClient<typeof Organizat
options?: CallOptions | undefined,
): Promise<UpdateOrganizationResponse> {
if (!request.organizationId) {
throw new ConnectError("id is required", Code.InvalidArgument);
throw new ConnectError("organizationId is required", Code.InvalidArgument);
}
if (!request.name) {
throw new ConnectError("name is required", Code.InvalidArgument);
Expand All @@ -97,7 +97,7 @@ export class JsonRpcOrganizationClient implements PromiseClient<typeof Organizat
options?: CallOptions | undefined,
): Promise<DeleteOrganizationResponse> {
if (!request.organizationId) {
throw new ConnectError("id is required", Code.InvalidArgument);
throw new ConnectError("organizationId is required", Code.InvalidArgument);
}
await getGitpodService().server.deleteTeam(request.organizationId);
return new DeleteOrganizationResponse();
Expand All @@ -108,7 +108,7 @@ export class JsonRpcOrganizationClient implements PromiseClient<typeof Organizat
options?: CallOptions | undefined,
): Promise<GetOrganizationInvitationResponse> {
if (!request.organizationId) {
throw new ConnectError("id is required", Code.InvalidArgument);
throw new ConnectError("organizationId is required", Code.InvalidArgument);
}
const result = await getGitpodService().server.getGenericInvite(request.organizationId);
return new GetOrganizationInvitationResponse({
Expand All @@ -134,7 +134,7 @@ export class JsonRpcOrganizationClient implements PromiseClient<typeof Organizat
options?: CallOptions | undefined,
): Promise<ResetOrganizationInvitationResponse> {
if (!request.organizationId) {
throw new ConnectError("id is required", Code.InvalidArgument);
throw new ConnectError("organizationId is required", Code.InvalidArgument);
}
const newInvite = await getGitpodService().server.resetGenericInvite(request.organizationId);
return new ResetOrganizationInvitationResponse({
Expand All @@ -147,7 +147,7 @@ export class JsonRpcOrganizationClient implements PromiseClient<typeof Organizat
options?: CallOptions | undefined,
): Promise<ListOrganizationMembersResponse> {
if (!request.organizationId) {
throw new ConnectError("id is required", Code.InvalidArgument);
throw new ConnectError("organizationId is required", Code.InvalidArgument);
}
const result = await getGitpodService().server.getTeamMembers(request.organizationId);
return new ListOrganizationMembersResponse({
Expand All @@ -160,7 +160,7 @@ export class JsonRpcOrganizationClient implements PromiseClient<typeof Organizat
options?: CallOptions | undefined,
): Promise<UpdateOrganizationMemberResponse> {
if (!request.organizationId) {
throw new ConnectError("id is required", Code.InvalidArgument);
throw new ConnectError("organizationId is required", Code.InvalidArgument);
}
if (!request.userId) {
throw new ConnectError("userId is required", Code.InvalidArgument);
Expand All @@ -181,7 +181,7 @@ export class JsonRpcOrganizationClient implements PromiseClient<typeof Organizat
options?: CallOptions | undefined,
): Promise<DeleteOrganizationMemberResponse> {
if (!request.organizationId) {
throw new ConnectError("id is required", Code.InvalidArgument);
throw new ConnectError("organizationId is required", Code.InvalidArgument);
}
if (!request.userId) {
throw new ConnectError("userId is required", Code.InvalidArgument);
Expand All @@ -195,7 +195,7 @@ export class JsonRpcOrganizationClient implements PromiseClient<typeof Organizat
options?: CallOptions | undefined,
): Promise<GetOrganizationSettingsResponse> {
if (!request.organizationId) {
throw new ConnectError("id is required", Code.InvalidArgument);
throw new ConnectError("organizationId is required", Code.InvalidArgument);
}
const result = await getGitpodService().server.getOrgSettings(request.organizationId);
return new GetOrganizationSettingsResponse({
Expand All @@ -208,11 +208,11 @@ export class JsonRpcOrganizationClient implements PromiseClient<typeof Organizat
options?: CallOptions | undefined,
): Promise<UpdateOrganizationSettingsResponse> {
if (!request.organizationId) {
throw new ConnectError("id is required", Code.InvalidArgument);
throw new ConnectError("organizationId is required", Code.InvalidArgument);
}
await getGitpodService().server.updateOrgSettings(request.organizationId, {
workspaceSharingDisabled: request.settings?.workspaceSharingDisabled,
defaultWorkspaceImage: request.settings?.defaultWorkspaceImage,
workspaceSharingDisabled: request?.workspaceSharingDisabled,
defaultWorkspaceImage: request?.defaultWorkspaceImage,
});
return new UpdateOrganizationSettingsResponse();
}
Expand Down
16 changes: 8 additions & 8 deletions components/dashboard/src/service/json-rpc-workspace-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@ import { WorkspaceInstance } from "@gitpod/gitpod-protocol";

export class JsonRpcWorkspaceClient implements PromiseClient<typeof WorkspaceService> {
async getWorkspace(request: PartialMessage<GetWorkspaceRequest>): Promise<GetWorkspaceResponse> {
if (!request.id) {
throw new ConnectError("id is required", Code.InvalidArgument);
if (!request.workspaceId) {
throw new ConnectError("workspaceId is required", Code.InvalidArgument);
}
const info = await getGitpodService().server.getWorkspace(request.id);
const info = await getGitpodService().server.getWorkspace(request.workspaceId);
const workspace = converter.toWorkspace(info);
const result = new GetWorkspaceResponse();
result.item = workspace;
result.workspace = workspace;
return result;
}

Expand All @@ -38,11 +38,11 @@ export class JsonRpcWorkspaceClient implements PromiseClient<typeof WorkspaceSer
throw new ConnectError("signal is required", Code.InvalidArgument);
}
if (request.workspaceId) {
const resp = await this.getWorkspace({ id: request.workspaceId });
if (resp.item?.status) {
const resp = await this.getWorkspace({ workspaceId: request.workspaceId });
if (resp.workspace?.status) {
const response = new WatchWorkspaceStatusResponse();
response.workspaceId = resp.item.id;
response.status = resp.item.status;
response.workspaceId = resp.workspace.id;
response.status = resp.workspace.status;
yield response;
}
}
Expand Down
10 changes: 5 additions & 5 deletions components/dashboard/src/start/StartWorkspace.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,14 +272,14 @@ export default class StartWorkspace extends React.Component<StartWorkspaceProps,
const { workspaceId } = this.props;
try {
const request = new GetWorkspaceRequest();
request.id = workspaceId;
request.workspaceId = workspaceId;
const response = await workspaceClient.getWorkspace(request);
if (response.item?.status?.instanceId) {
if (response.workspace?.status?.instanceId) {
this.setState((s) => ({
workspace: response.item,
startedInstanceId: s.startedInstanceId || response.item?.status?.instanceId, // note: here's a potential mismatch between startedInstanceId and instance.id. TODO(gpl) How to handle this?
workspace: response.workspace,
startedInstanceId: s.startedInstanceId || response.workspace?.status?.instanceId, // note: here's a potential mismatch between startedInstanceId and instance.id. TODO(gpl) How to handle this?
}));
this.onWorkspaceUpdate(response.item);
this.onWorkspaceUpdate(response.workspace);
}
} catch (error) {
console.error(error);
Expand Down
10 changes: 6 additions & 4 deletions components/dashboard/src/teams/TeamSettings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { organizationClient } from "../service/public-api";
import { gitpodHostUrl } from "../service/service";
import { useCurrentUser } from "../user-context";
import { OrgSettingsPage } from "./OrgSettingsPage";
import { ErrorCode } from "@gitpod/gitpod-protocol/lib/messaging/error";

export default function TeamSettingsPage() {
const user = useCurrentUser();
Expand Down Expand Up @@ -204,7 +205,6 @@ function OrgSettingsForm(props: { org?: Organization; isOwner: boolean }) {
<form
onSubmit={(e) => {
e.preventDefault();
// handleUpdateTeamSettings({ defaultWorkspaceImage });
}}
>
{props.org && (
Expand Down Expand Up @@ -282,7 +282,7 @@ function WorkspaceImageButton(props: {
};
}

const image = props.settings?.defaultWorkspaceImage ?? props.defaultWorkspaceImage ?? "";
const image = props.settings?.defaultWorkspaceImage || props.defaultWorkspaceImage || "";
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


const descList = useMemo(() => {
const arr: ReactNode[] = [<span>Default image</span>];
Expand Down Expand Up @@ -348,7 +348,7 @@ interface OrgDefaultWorkspaceImageModalProps {

function OrgDefaultWorkspaceImageModal(props: OrgDefaultWorkspaceImageModalProps) {
const [errorMsg, setErrorMsg] = useState("");
const [defaultWorkspaceImage, setDefaultWorkspaceImage] = useState(props.settings?.defaultWorkspaceImage ?? "");
const [defaultWorkspaceImage, setDefaultWorkspaceImage] = useState(props.settings?.defaultWorkspaceImage || "");
const updateTeamSettings = useUpdateOrgSettingsMutation();

const handleUpdateTeamSettings = useCallback(
Expand All @@ -360,7 +360,9 @@ function OrgDefaultWorkspaceImageModal(props: OrgDefaultWorkspaceImageModalProps
});
props.onClose();
} catch (error) {
console.error(error);
if (!ErrorCode.isUserError(error["code"])) {
console.error(error);
}
setErrorMsg(error.message);
}
},
Expand Down
2 changes: 1 addition & 1 deletion components/gitpod-db/src/team-db.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export interface TeamDB extends TransactionalDB<TeamDB> {
deleteTeam(teamId: string): Promise<void>;

findOrgSettings(teamId: string): Promise<OrganizationSettings | undefined>;
setOrgSettings(teamId: string, settings: Partial<OrganizationSettings>): Promise<void>;
setOrgSettings(teamId: string, settings: Partial<OrganizationSettings>): Promise<OrganizationSettings>;

hasActiveSSO(organizationId: string): Promise<boolean>;
}
23 changes: 7 additions & 16 deletions components/gitpod-db/src/typeorm/team-db-impl.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

const repo = await this.getOrgSettingsRepo();
const team = await repo.findOne({ where: { orgId, deleted: false } });
const update: Partial<OrganizationSettings> = {
defaultWorkspaceImage: settings.defaultWorkspaceImage,
workspaceSharingDisabled: settings.workspaceSharingDisabled,
};
// Set to null if defaultWorkspaceImage is empty string, so that it can fallback to default image
if (update.defaultWorkspaceImage?.trim() === "") {
update.defaultWorkspaceImage = null;
}
if (!team) {
await repo.insert({
...update,
return await repo.save({
...settings,
orgId,
});
} else {
await repo.save({
...team,
...update,
});
}
return await repo.save({
...team,
...settings,
});
}

public async hasActiveSSO(organizationId: string): Promise<boolean> {
Expand Down
38 changes: 7 additions & 31 deletions components/gitpod-protocol/src/messaging/error.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
*/

import { scrubber } from "../util/scrubbing";
import { Status } from "nice-grpc-common";

export class ApplicationError extends Error {
constructor(public readonly code: ErrorCode, message: string, public readonly data?: any) {
Expand All @@ -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"]);
Copy link
Member Author

Choose a reason for hiding this comment

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

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

}

export async function notFoundToUndefined<T>(p: Promise<T>): Promise<T | undefined> {
Expand All @@ -37,41 +36,18 @@ export namespace ApplicationError {
throw e;
}
}

export function fromGRPCError(e: any, data?: any): ApplicationError {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

// Argument e should be ServerErrorResponse
// But to reduce dependency requirement, we use Error here

// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
return new ApplicationError(categorizeRPCError(e.code), e.message, data);
}

export function categorizeRPCError(code?: Status): ErrorCode {
// Mostly align to https://github.com/gitpod-io/gitpod/blob/ef95e6f3ca0bf314c40da1b83251423c2208d175/components/public-api-server/pkg/proxy/errors.go#L25
switch (code) {
case Status.INVALID_ARGUMENT:
return ErrorCodes.BAD_REQUEST;
case Status.UNAUTHENTICATED:
return ErrorCodes.NOT_AUTHENTICATED;
case Status.PERMISSION_DENIED:
return ErrorCodes.PERMISSION_DENIED; // or UserBlocked
case Status.NOT_FOUND:
return ErrorCodes.NOT_FOUND;
case Status.ALREADY_EXISTS:
return ErrorCodes.CONFLICT;
case Status.FAILED_PRECONDITION:
return ErrorCodes.PRECONDITION_FAILED;
case Status.RESOURCE_EXHAUSTED:
return ErrorCodes.TOO_MANY_REQUESTS;
}
return ErrorCodes.INTERNAL_SERVER_ERROR;
}
}

export namespace ErrorCode {
export function isUserError(code: number | ErrorCode) {
return code >= 400 && code < 500;
}
export function is(code: any): code is ErrorCode {
if (typeof code !== "number") {
return false;
}
return Object.values(ErrorCodes).includes(code as ErrorCode);
}
}

export type ErrorCode = typeof ErrorCodes[keyof typeof ErrorCodes];
Expand Down
Loading
Loading