Skip to content

decouple gp env from theia #3569

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 2 commits into from
Mar 29, 2021
Merged

Conversation

akosyakov
Copy link
Member

@akosyakov akosyakov commented Mar 24, 2021

What it does

How to test

  • test both Theia and VS Code
  • get: should return exact and wildcard matches, should not return unmatched i.e. wildcards or other repos
  • set/delete: should create, updated or delete based on exact matches, should not mutate unmatched i.e. wildcards or other repos

@akosyakov akosyakov force-pushed the akosyakov/gp-support-gp-env-by-3162 branch 14 times, most recently from 8a02196 to ade6376 Compare March 25, 2021 13:22
@akosyakov akosyakov force-pushed the akosyakov/gp-support-gp-env-by-3162 branch from fa9cace to 16a56e0 Compare March 25, 2021 19:38
@akosyakov akosyakov marked this pull request as ready for review March 25, 2021 19:53
@@ -176,11 +218,14 @@ export namespace ScopedResourceGuard {

export const SNAPSHOT_WORKSPACE_SUBJECT_ID_PREFIX = 'ws-'

export interface ResourceScope {
kind: GuardedResourceKind;
export interface ResourceScope<K extends GuardedResourceKind = GuardedResourceKind> {
Copy link
Member

Choose a reason for hiding this comment

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

❤️

const envvar: UserEnvVar = {
...variable,
id: variable.id || uuidv4(),
userId: user.id,
};
await this.guardAccess({ kind: 'envVar', subject: envvar }, typeof variable.id === 'string' ? 'update' : 'create');
Copy link
Member

Choose a reason for hiding this comment

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

see below

await this.userDB.setEnvVar(envvar);
}

async deleteEnvVar(variable: UserEnvVarValue): Promise<void> {
// Note: this operation is per-user only, hence needs no resource guard
const user = this.checkUser("deleteEnvVar");

if (!variable.id && variable.name && variable.repositoryPattern) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be the same/a similar check as above. Would it make sense to extract those into one method isUpdate?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would prefer to keep code like that, since I'm not sure about client expectation when id is provided for delete.

Ideally I would remove id completely and use (name, repositoryPattern) as a primary key.

@geropl
Copy link
Member

geropl commented Mar 26, 2021

/werft run

👍 started the job as gitpod-build-akosyakov-gp-support-gp-env-by-3162.22

will test now...

@geropl
Copy link
Member

geropl commented Mar 26, 2021

Tested, and works as advertised. Not approving yet due to the ongoing discussions above. ☝️

Copy link
Contributor

@JanKoehnlein JanKoehnlein left a comment

Choose a reason for hiding this comment

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

LGTM

@akosyakov akosyakov force-pushed the akosyakov/gp-support-gp-env-by-3162 branch from 16a56e0 to 09b2dcd Compare March 27, 2021 09:17
@@ -35,6 +36,13 @@ var gitTokenValidator = &cobra.Command{
Args: cobra.ExactArgs(0),
Hidden: true,
Run: func(cmd *cobra.Command, args []string) {
log.SetOutput(io.Discard)
Copy link
Member Author

@akosyakov akosyakov Mar 27, 2021

Choose a reason for hiding this comment

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

@JanKoehnlein Does it make sense to you? Otherwise I am not sure where everything get logged?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it makes sense. I am just not sure whether we should use another file. The credential helper could still have the same file open for appending, as it starts the token validator in background and then releases the file on exit. Anyway, this shouldn't be a problem on Linux.

@akosyakov
Copy link
Member Author

akosyakov commented Mar 29, 2021

/werft run

👍 started the job as gitpod-build-akosyakov-gp-support-gp-env-by-3162.24

Copy link
Contributor

@JanKoehnlein JanKoehnlein left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@JanKoehnlein JanKoehnlein left a comment

Choose a reason for hiding this comment

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

In VS Code

$ gp env BAR=BAR
cannot set BAR: jsonrpc2: code 403 message: operation not permitted: missing create permission on envVar

@akosyakov akosyakov force-pushed the akosyakov/gp-support-gp-env-by-3162 branch from 09b2dcd to 3089e19 Compare March 29, 2021 08:16
@JanKoehnlein
Copy link
Contributor

When I try to unset a variable with a wildcard scope using gp env, I get an unhelpful error message

cannot unset foo: jsonrpc2: code 404 message: Missing ID field

Apart from that it works fin now, in Theia and VS Code.

@JanKoehnlein JanKoehnlein self-requested a review March 29, 2021 11:18
Copy link
Contributor

@JanKoehnlein JanKoehnlein left a comment

Choose a reason for hiding this comment

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

LGTM

@akosyakov akosyakov force-pushed the akosyakov/gp-support-gp-env-by-3162 branch from 3089e19 to 3ba02c2 Compare March 29, 2021 11:40
@akosyakov akosyakov force-pushed the akosyakov/gp-support-gp-env-by-3162 branch from 3ba02c2 to 2adaa06 Compare March 29, 2021 11:53
@akosyakov
Copy link
Member Author

akosyakov commented Mar 29, 2021

/werft run

👍 started the job as gitpod-build-akosyakov-gp-support-gp-env-by-3162.29

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

Successfully merging this pull request may close these issues.

[gp] Support gp env by talking to server directly
3 participants