[api, server, dashboard] Cleanup UpdateOrganizationSettings API #20603
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
geropl
force-pushed
the
gpl/org-settings-refresh
branch
from
183b555 to
24f38b4
Compare
geropl
commented
Feb 14, 2025
components/dashboard/src/service/json-rpc-organization-client.ts
Outdated
Show resolved
Hide resolved
geropl
commented
Feb 14, 2025
| @@ -25,34 +25,34 @@ export class DBOrgSettings implements OrganizationSettings { | |||
| workspaceSharingDisabled?: boolean; | |||
|
|
|||
| @Column("varchar", { nullable: true }) | |||
| defaultWorkspaceImage?: string | null; | |||
| defaultWorkspaceImage?: string; | |||
There was a problem hiding this comment.
The null is treated as special value by typeorm to clear the column.
This is the only place we did this, and it's also unnecessary: We treat the default value "" the same anyway. 🤷 So we can get rid of that complexity.
geropl
commented
Feb 14, 2025
geropl
commented
Feb 14, 2025
| @@ -1059,7 +1059,45 @@ export class PublicAPIConverter { | |||
| } | |||
| } | |||
|
|
|||
| fromTimeoutSettings(timeoutSettings: TimeoutSettings): TimeoutSettingsProtocol { | |||
| fromOrganizationSettings(settings: PlainMessage<OrganizationSettings>): OrganizationSettingsProtocol { | |||
There was a problem hiding this comment.
This is the field translation, untangled from validation etc. from the organization-service-api.ts handler.
geropl
commented
Feb 14, 2025
| if (req.updateRestrictedEditorNames) { | ||
| update.restrictedEditorNames = req.restrictedEditorNames; | ||
| } else if (req.restrictedEditorNames.length > 0) { | ||
| const settings = req.settings; |
There was a problem hiding this comment.
The code here was split into three things:
- gRPC validation (stayed here)
- general field validation (went into
organization-service.ts - field translation (went to
public-api-converter.ts)
geropl
commented
Feb 14, 2025
| @@ -551,6 +550,22 @@ export class OrganizationService { | |||
| } | |||
| } | |||
|
|
|||
| if (settings.maxParallelRunningWorkspaces !== undefined) { | |||
There was a problem hiding this comment.
From the -api.ts handler.
geropl
force-pushed
the
gpl/org-settings-refresh
branch
3 times, most recently
from
e90ca11 to
fa14fac
Compare
Tool: gitpod/catfood.gitpod.cloud
geropl
force-pushed
the
gpl/org-settings-refresh
branch
from
f36f3a1 to
d32f465
Compare
Tool: gitpod/catfood.gitpod.cloud
geropl
force-pushed
the
gpl/org-settings-refresh
branch
from
d32f465 to
d829633
Compare
Tool: gitpod/catfood.gitpod.cloud
|
/unhold |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Follow-up for: #20577
This PR cleans up the UpdateOrganizationSettings API shapes and related handlers and dashboard client.
The changes keep the current inconsistencies w.r.t. duplicate shapes and different field IDs, but maintains (or strengthens) the compatibility on the field name level. This allows us to get rid of a ton of duplicate code/field handling.
Related Issue(s)
Fixes #
How to test
Documentation
Preview status
gitpod:summary
Build Options
Build
Run the build with werft instead of GHA
Run Leeway with
--dont-testPublish
Installer
Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
If enabled this will build
install/previewIf enabled this will create the environment on GCE infra
Saves cost. Untick this only if you're really sure you need a non-preemtible machine.
Valid options are
all,workspace,webapp,ide,jetbrains,vscode,ssh. If enabled,with-previewandwith-large-vmwill be enabled./hold