-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[public-api] Migrate envvarService #19067
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
Conversation
be9c16d
to
a667faa
Compare
@akosyakov could you review again 🙏 |
message ResolveWorkspaceEnvironmentVariablesRequest { string workspace_id = 1; } | ||
|
||
message ResolveWorkspaceEnvironmentVariablesResponse { | ||
message EnvironmentVariable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generally looks good, not sure about it, maybe it is find to have it on global level?
@jeanp413 it looks ok, you can consider moving proto file and generated classes in another PR, to make this PR small, I will approve it to unblock. |
@akosyakov moved the proto files to another PR #19085 |
/gh run recreate-vm Comment triggered a workflow runStarted workflow run: 6900579887
|
Tested and it's working |
@jeanp413 open for a review? |
repositoryPattern: "", | ||
}; | ||
|
||
await this.envVarService.deleteUserEnvVar(context.user.id, context.user.id, variable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is not enough to pass id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's needed for old api resource guard validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, ignore my noise on gRPC implementation here, when we get rid of JSON-RPC we can simplify, don't want to block with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already updated a couple 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left comment on gRPC implementation. I think at least we should fix error propagation, logic looks unnecessary complicated, but it seems because of underlying application service design. I wonder whether we can improve it, if it is too much work let's go with what we have for now.
3e2ec68
to
81d1654
Compare
Add some nit changes cd19ba3 ❌ Failed to update user env, checked that list API has respond with id. Going to update code ![]() |
I tried to create workspace with another ORG, it seems new workspace still includes previous ORG envs (the same repo) |
org env? you mean user env or project env? |
I mean org project env. Seems something wrong in our code. Project
|
But ☝️ not relates to this PR's changes, tested all good. |
Let's wait preview env to validate updateUserEnv fixing. Once it's ok, we are good to go |
😆 |
@@ -1830,11 +1830,11 @@ export class GitpodServerImpl implements GitpodServerWithTracing, Disposable { | |||
const user = await this.checkAndBlockUser("setEnvVar"); | |||
const userEnvVars = await this.envVarService.listUserEnvVars(user.id, user.id); | |||
if (userEnvVars.find((v) => v.name == variable.name && v.repositoryPattern == variable.repositoryPattern)) { | |||
return this.envVarService.updateUserEnvVar(user.id, user.id, variable, (envvar: UserEnvVar) => { | |||
await this.envVarService.updateUserEnvVar(user.id, user.id, variable, (envvar: UserEnvVar) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we update userEnv with value
and scope
changed, server will call addUserEnvVar
even we have envId
passed.
Not blocker as it's just like before 😌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, intended
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eae9783
to
47317a8
Compare
Description
Summary generated by Copilot
🤖 Generated by Copilot at 2d8b9cb
This pull request adds a new public API service for managing user environment variables for workspaces. It defines the service and its messages using Protobuf, generates client and server code for gRPC and Connect in Go and TypeScript, and implements the service handler and client in the server and dashboard components. It also refactors the dashboard component for environment variables to use the new public API service.
Related Issue(s)
Fixes #
How to test
org
,configuration
anduser
levelorg
, envs inorg
+user
should be in workspaceorg
+configuration
, envs inorg
+configuration
+user
should be in workspaceorg
, there should only haveuser
envs in workspaceDocumentation
Preview status
Gitpod was successfully deployed to your preview environment.
Build Options
Build
Run the build with werft instead of GHA
Run Leeway with
--dont-test
Publish
Installer
Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
If enabled this will build
install/preview
If enabled this will create the environment on GCE infra
Valid options are
all
,workspace
,webapp
,ide
,jetbrains
,vscode
,ssh
. If enabled,with-preview
andwith-large-vm
will be enabled./hold