-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[public-api] Add AuthProviderService service #19008
Conversation
components/public-api/gitpod/experimental/v2/authprovider.proto
Outdated
Show resolved
Hide resolved
components/public-api/gitpod/experimental/v2/authprovider.proto
Outdated
Show resolved
Hide resolved
components/public-api/gitpod/experimental/v2/authprovider.proto
Outdated
Show resolved
Hide resolved
components/public-api/gitpod/experimental/v2/authprovider.proto
Outdated
Show resolved
Hide resolved
components/public-api/gitpod/experimental/v2/authprovider.proto
Outdated
Show resolved
Hide resolved
components/public-api/gitpod/experimental/v2/authprovider.proto
Outdated
Show resolved
Hide resolved
8441e9e
to
f222e04
Compare
f8f6da6
to
a63c62c
Compare
} | ||
const clientId = request.clientId; | ||
const clientSecret = request.clientSecret; | ||
if (!clientId || typeof clientSecret === "undefined") { |
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.
we don't require both? it is rather at least one of them should be present
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 used following pattern here:
gitpod/components/server/src/api/organization-service-api.ts
Lines 244 to 265 in c33b555
const update: Partial<ProtocolOrganizationSettings> = {}; | |
if (typeof req.workspaceSharingDisabled === "boolean") { | |
update.workspaceSharingDisabled = req.workspaceSharingDisabled; | |
} | |
const hasDefaultWorkspaceImage = typeof req.defaultWorkspaceImage === "string"; | |
const resetDefaultWorkspaceImage = req.resetMask?.paths.includes("defaultWorkspaceImage"); | |
if (hasDefaultWorkspaceImage && resetDefaultWorkspaceImage) { | |
throw new ConnectError( | |
"defaultWorkspaceImage cannot be set and reset at the same time", | |
Code.InvalidArgument, | |
); | |
} | |
if (resetDefaultWorkspaceImage) { | |
update.defaultWorkspaceImage = null; | |
} else if (hasDefaultWorkspaceImage) { | |
update.defaultWorkspaceImage = req.defaultWorkspaceImage; | |
} | |
if (Object.keys(update).length === 0) { | |
throw new ConnectError("nothing to update", Code.InvalidArgument); | |
} |
not sure whether we want to align
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.
This would mean, we need to change the Dashboard part and retest the behavior. I'd rather avoid.
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.
- We don't build API for one client (dashboard).
- If dashboard already provides both values it should not be an issue anyway?
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.
Ah, now I got you.
we don't require both? it is rather at least one of them should be present
wasn't clear from that.
So, even if Dashboard is always sending the tuple, we may allow for adjusting a single attribute here as well, right? We can change that. Just the focus wasn't on changing existing behavior for the migration phase.
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.
resolved in fc3be82
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.
pre approved, but please to align on contract, and unnecessary checks before mergin 🙏
allow to update clientId or clientSecret separately.
@akosyakov, I've address the comments and added tests. Going to test in preview and proceed afterwards. |
3235bb8
to
3055c58
Compare
/unhold |
Description
Depends on #19041, which depends on #19017
This PR adds and implements public-api service for AuthProviders
Summary generated by Copilot
🤖 Generated by Copilot at 55bd7e7
This pull request adds the AuthProviderService to the experimental API, which is a service for managing authentication providers. It generates the code for the service using different protocols and plugins, such as gRPC, Connect, and protoc-gen-go-grpc. It also adds the proto file that defines the service and its messages and enums.
Related Issue(s)
Fixes #
How to test
Documentation
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