Skip to content

[server] Use content-service #3288

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

Jump to bottom
Merged
merged 1 commit into from
Mar 23, 2021
Merged

[server] Use content-service #3288

merged 1 commit into from
Mar 23, 2021

Conversation

corneliusludmann
Copy link
Contributor

@corneliusludmann corneliusludmann commented Feb 26, 2021 •
edited
Loading

This PR removes the dependency to the remote storage configuration from the server component. Instead, it uses the content-server component.

Fixes #3144

How to test

The following functions are affected and should be tested with MinIO and GCP storage:

  • Installing a VSCode extension (for the workspace not the user to have it for the next step)
  • Start a workspace with a previous installed VSCode extension (just stop and restart the workspace from prev. step)
  • Download workspace data from the dashboard
  • Delete a workspace (after pressing the delete button change the soft deletion time in the DB, e.g update d_b_workspace set softDeletedTime = '2020-01-01 01:30:30';, here for all workspaces; wait for max. 30 min. to let the GC run (see server log), verify in MinIO or GCP storage that the workspace backup is gone)
  • Delete a user account (verify in MinIO or GCP storage that the user bucket is gone), should be tested with a non-empty user bucket!

@corneliusludmann corneliusludmann force-pushed the corneliusludmann/server-move-all-3144 branch 10 times, most recently from 95c86b1 to 15d59d9 Compare February 27, 2021 19:09
@corneliusludmann corneliusludmann marked this pull request as ready for review February 27, 2021 19:28
@corneliusludmann corneliusludmann changed the title WIP – [server] Use content-service [server] Use content-service Feb 27, 2021
@corneliusludmann
Copy link
Contributor Author

@csweichel: This PR is ready for a review. Probably too late for the March release, though.

csweichel
csweichel reviewed Mar 1, 2021
View reviewed changes
Copy link
Contributor

@csweichel csweichel left a comment

Choose a reason for hiding this comment

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

It's nice to see this stuff finally move out of server

@corneliusludmann corneliusludmann force-pushed the corneliusludmann/server-move-all-3144 branch 2 times, most recently from e03b9e7 to 65af066 Compare March 1, 2021 10:27
@csweichel csweichel mentioned this pull request Mar 1, 2021
Closed
@csweichel csweichel mentioned this pull request Mar 12, 2021
Merged
@corneliusludmann corneliusludmann force-pushed the corneliusludmann/server-move-all-3144 branch 3 times, most recently from e64d463 to 43431cf Compare March 13, 2021 21:00
@corneliusludmann corneliusludmann mentioned this pull request Mar 14, 2021
Closed
@corneliusludmann corneliusludmann force-pushed the corneliusludmann/server-move-all-3144 branch 2 times, most recently from 9bb7e7e to 07bc6c0 Compare March 14, 2021 11:48
@corneliusludmann corneliusludmann mentioned this pull request Mar 15, 2021
Merged
corneliusludmann
corneliusludmann commented Mar 15, 2021
View reviewed changes
@corneliusludmann corneliusludmann force-pushed the corneliusludmann/server-move-all-3144 branch 2 times, most recently from a073584 to 7edd238 Compare March 18, 2021 13:17
geropl
geropl reviewed Mar 19, 2021
View reviewed changes
geropl
geropl reviewed Mar 19, 2021
View reviewed changes
components/server/src/container-module.ts Outdated
@@ -203,8 +166,16 @@ export const productionContainerModule = new ContainerModule((bind, unbind, isBo

bind(TermsProvider).toSelf().inSingletonScope();

const contentServiceClient = new ContentServiceClient("content-service:8080", grpc.credentials.createInsecure())
Copy link
Member

Choose a reason for hiding this comment

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

Maybe pedantic, but I'd love to see the address as part of the env with this value as default, even if not used yet by helm.

geropl
geropl reviewed Mar 19, 2021
View reviewed changes
components/server/src/env.ts
@@ -54,17 +54,6 @@ export class Env extends AbstractComponentEnv {

readonly sessionMaxAgeMs: number = Number.parseInt(process.env.SESSION_MAX_AGE_MS || '259200000' /* 3 days */, 10);

readonly storageClient: string = process.env.GITPOD_STORAGE_CLIENT || 'gcloud';
Copy link
Member

Choose a reason for hiding this comment

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

❤️

geropl
geropl reviewed Mar 19, 2021
View reviewed changes
components/server/src/storage/content-service-client.ts Outdated
}
});
});
try {
Copy link
Member

@geropl geropl Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is redundant. Directly awaiting the new Promise<Delete... is enough

geropl
geropl reviewed Mar 19, 2021
View reviewed changes
components/server/src/storage/content-service-client.ts Outdated
});
});
try {
await grcpPromise;
Copy link
Member

Choose a reason for hiding this comment

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

same as above.

geropl
geropl reviewed Mar 19, 2021
View reviewed changes
components/server/src/storage/content-service-client.ts Outdated
}
});
});
try {
Copy link
Member

Choose a reason for hiding this comment

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

basically same as above.

geropl
geropl reviewed Mar 19, 2021
View reviewed changes
components/server/src/storage/content-service-client.ts Outdated
}
});
});
try {
Copy link
Member

Choose a reason for hiding this comment

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

same as above

geropl
geropl reviewed Mar 19, 2021
View reviewed changes
components/server/src/storage/content-service-client.ts Outdated
}
});
});
try {
Copy link
Member

Choose a reason for hiding this comment

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

same as above

geropl
geropl reviewed Mar 19, 2021
View reviewed changes
components/server/src/storage/content-service-client.ts Outdated
}
});
});
try {
Copy link
Member

Choose a reason for hiding this comment

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

same as above

@csweichel
Copy link
Contributor

csweichel commented Mar 19, 2021 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-corneliusludmann-server-move-all-3144.83

csweichel
csweichel approved these changes Mar 19, 2021
View reviewed changes
Copy link
Contributor

@csweichel csweichel left a comment

Choose a reason for hiding this comment

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

Awesome. Thank you!

Works like it says on the tin.

@corneliusludmann corneliusludmann force-pushed the corneliusludmann/server-move-all-3144 branch from 7edd238 to ad72008 Compare March 23, 2021 13:57
@corneliusludmann corneliusludmann force-pushed the corneliusludmann/server-move-all-3144 branch from ad72008 to 06bc3e7 Compare March 23, 2021 19:48
@corneliusludmann corneliusludmann merged commit e56ed09 into main Mar 23, 2021
@corneliusludmann corneliusludmann deleted the corneliusludmann/server-move-all-3144 branch March 23, 2021 20:03
@akosyakov akosyakov mentioned this pull request Apr 28, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geropl geropl geropl left review comments

@csweichel csweichel csweichel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[server] Move all remote storage access to content-service
3 participants
@corneliusludmann @csweichel @geropl