-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Org Settings Page Header updates & cleanup #16850
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
started the job as gitpod-build-bmh-git-auth-cleanup.2 because the annotations in the pull request description changed |
/werft run with-preview=true 👍 started the job as gitpod-build-bmh-git-auth-cleanup.3 |
@@ -73,6 +77,12 @@ const infoMap: Record<AlertType, AlertInfo> = { | |||
icon: <Exclamation className="w-4 h-4"></Exclamation>, | |||
iconColor: "text-red-400", | |||
}, | |||
danger: { |
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.
Wanted the alert w/ a darker red background for modals. Open to alternatives here.
@@ -85,11 +90,10 @@ function OIDCClients(props: { organizationId: string }) { | |||
{modal?.mode === "edit" && <></>} | |||
{modal?.mode === "delete" && <></>} | |||
|
|||
<Heading2>OpenID Connect providers</Heading2> |
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.
@selfcontained, here we've to be clear what's actually being set up, i.e. be clear about client vs. provider. Technically, we are not setting up a provider, but configuring a client for external providers. As there can be more than one client for a IdP (provider like google
) we should avoid calling it provider here.
Also, there are ideas floating to make Gitpod an IdP, even if that would be just proxying other IdPs, having the roles clarified prevents from confusion.
Let's go with "OpenID Connect clients" 🙏🏻
|
||
export default function SSO() { | ||
const currentOrg = useCurrentOrg(); | ||
|
||
return <OrgSettingsPage>{currentOrg.data && <OIDCClients organizationId={currentOrg.data?.id} />}</OrgSettingsPage>; | ||
return ( | ||
<OrgSettingsPage title="SSO" subtitle="Configure OpenID Connect single sign-on providers."> |
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'd propose to drop "providers" from subtitle, and rather include the acronym used below. Maybe like this: Configure OpenID Connect (OIDC) single sign-on.
</div> | ||
<EmptyMessage | ||
title="No Git Integrations" | ||
subtitle="In addition to the default Git Providers you can authorize with a self-hosted instance of a provider." |
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.
Let's rephrase the subtitle to avoid confusion we have with Gitpod Self-Hosted vs. SaaS 🙈 With introducing Git Auth for organizations, we want to enable adding those per organization. I'd say even mentioning the "defaults" isn't relevant here, as encapsulation is what organizations are about.
What do you think of simplifying and reducing to "Configure Git Auth with GitHub or GitLab".
Sidebar: bitbucket.org is not supported 🙈 just the self-managed BBS
@@ -24,11 +24,14 @@ export default function GitAuth() { | |||
export const OrgSettingsPageWrapper: FunctionComponent = ({ children }) => { | |||
const currentOrg = useCurrentOrg(); | |||
|
|||
const title = "Git Integrations"; |
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.
🙏🏻 could we rename it to Git Auth
to emphasize that this feature is about authorization of Git commands (and APIs). AFAIR the integrations was just some middle ground as we had to incorporate the login thing into it, but here we can clarify on the expectations.
@@ -24,11 +24,14 @@ export default function GitAuth() { | |||
export const OrgSettingsPageWrapper: FunctionComponent = ({ children }) => { | |||
const currentOrg = useCurrentOrg(); | |||
|
|||
const title = "Git Integrations"; | |||
const subtitle = "Configure Git integrations for self-managed instances of GitLab, GitHub, or Bitbucket."; |
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.
Let's go with something like Configure Git Auth for GitLab, or Github.
Minus self-managed
as this is to support SaaS (gitlab.com + github.com)
</div> | ||
<EmptyMessage | ||
title="No OIDC providers" | ||
subtitle="Enter the client ID, client secret, and other information issued by your identity provider, to enable single sign-on for your organization." |
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'm curious what the empty message should be good for. The general point of "enable single sign-on for your organization" is perfect!
On this view, one cannot enter anything, thus the call to enter details demands too much.
I think what really is missing here is the mentioning of supported IdPs.
WDYT of something like this?
Enable single sign-on for your organization using an external identity provider (IdP) service that supports the OpenID Connect (OIDC) standard, such as Google.
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.
Thanks for taking care of this! 🙏🏻
Description
Some cleanup around the Org Settings pages.
Updates the Header/Subheader for the Org Settings pages
Each page now updates the main head accordingly
Cleans up displaying errors/warnings in a Modal in the Git Integrations flow
<ModalFooter error="error message" warning="warning message" />
error
takes precedence.<Alert type="danger"/>
to get the darker alert style. Open to alternatives here, cc @gtsiolisRelated Issue(s)
Fixes #15995
How to test
Release Notes
Documentation
Build Options:
Run the build with werft instead of GHA
Run Leeway with
--dont-test
Publish Options
Installer Options
Add desired feature flags to the end of the line above, space separated
Preview Environment Options:
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