Skip to content

[server] Remove superfluous config field builtinAuthProvidersConfigured #9612

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 1 commit into from
Apr 28, 2022

Conversation

geropl
Copy link
Member

@geropl geropl commented Apr 28, 2022

Description

Related Issue(s)

Fixes #

How to test

Release Notes

NONE

Documentation

@geropl geropl requested a review from a team April 28, 2022 09:05
@geropl geropl requested a review from andrew-farries April 28, 2022 09:05
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Apr 28, 2022
@geropl geropl mentioned this pull request Apr 28, 2022
53 tasks
Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

LGTM. Left a comment to think about the implications of changing config across component boundaries, up to you if you want to follow it.

@@ -18,10 +18,11 @@ import { log, LogrusLogLevel } from "@gitpod/gitpod-protocol/lib/util/logging";
import { filePathTelepresenceAware } from "@gitpod/gitpod-protocol/lib/env";

export const Config = Symbol("Config");
export type Config = Omit<ConfigSerialized, "hostUrl" | "chargebeeProviderOptionsFile"> & {
Copy link
Member

Choose a reason for hiding this comment

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

A possible risk with this change across boundaries is that a revert of just server may be broken against Installer config provided.

Normally, when removing config it's first made unused by the consumer (server) and then removed in later releases from the config specified.

Copy link
Member Author

Choose a reason for hiding this comment

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

A possible risk with this change across boundaries is that a revert of just server may be broken against Installer config provided.

In theory yes, but we 1) do not validate server config (so won't break if it's passed nonetheless), and 2) it's always overridden here.

@easyCZ easyCZ self-assigned this Apr 28, 2022
@roboquat roboquat merged commit 263ad01 into main Apr 28, 2022
@roboquat roboquat deleted the gpl/cfg-builtinauths branch April 28, 2022 09:12
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/XS team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants