Skip to content
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.

Sign up for GitHub

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

fix: cluster-wizard UI & oidc-scopes #3209

Merged
merged 13 commits into from
Aug 27, 2024
Merged

fix: cluster-wizard UI & oidc-scopes #3209

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a5509d2
fix: improved margin of oidc switch
chriskari Aug 20, 2024
ea9e708
fix: display multiple scopes in cluster review
chriskari Aug 20, 2024
4049dd7
fix: improved css of first wizard step
chriskari Aug 21, 2024
259877e
fix: display multiple scopes in cluster review
chriskari Aug 21, 2024
62d57c2
fix: wizard and dialog size
chriskari Aug 21, 2024
ab14770
fix: further css adjustments
chriskari Aug 21, 2024
2440f39
fix: further css adjustments
chriskari Aug 22, 2024
5126b78
fix: further css adjustments
chriskari Aug 22, 2024
bb9b93a
fix: form ui adjustments
chriskari Aug 23, 2024
b16ccde
fix: list input instead of formfield for scopes
chriskari Aug 25, 2024
629b564
test: adjust unit test to previous changes
chriskari Aug 25, 2024
a0c6004
fix: openid scope always present
chriskari Aug 25, 2024
41abccf
fix: openid scope always present
chriskari Aug 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/Clusters/components/AddClusterDialog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export function AddClusterDialog() {
return (
<Dialog
open={showWizard}
className="wizard-dialog"
className="add-cluster-wizard-dialog"
headerText={t('clusters.add.title')}
onAfterClose={() => setShowWizard(false)}
>
Expand Down
29 changes: 18 additions & 11 deletions src/components/Clusters/components/AddClusterWizard.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,13 @@ export function AddClusterWizard({ kubeconfig, setKubeconfig, config }) {
const setIsFormOpen = useSetRecoilState(isFormOpenState);

useEffect(() => {
const contentContainer = document
.getElementsByTagName('ui5-wizard')[0]
?.shadowRoot?.querySelectorAll('.ui5-wiz-content-item-wrapper')[
selected - 1
];
const wizard = document.getElementsByTagName('ui5-wizard')[0];
const wizardContent = wizard?.shadowRoot?.querySelector('.ui5-wiz-content');

const contentContainer = wizardContent?.querySelectorAll(
'.ui5-wiz-content-item-wrapper',
)[selected - 1];

if (contentContainer) {
contentContainer.style['background-color'] = 'transparent';
contentContainer.style['padding'] = '0';
Expand Down Expand Up @@ -93,6 +95,11 @@ export function AddClusterWizard({ kubeconfig, setKubeconfig, config }) {

const onComplete = () => {
try {
kubeconfig?.users.forEach(user => {
if (!user?.user?.exec?.args?.includes('--oidc-extra-scope=openid')) {
user?.user?.exec?.args?.push('--oidc-extra-scope=openid');
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a specific reason why you don't want to go this way?

https://github.com/kyma-project/busola/pull/3289/files

Busola is trying to mimic the behavior of the kubelogin command, which does not require openid to be passed as --oidc-extra-scope. Imho you should implement it the same way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Christian, thanks for your review and contribution.
Yes you are correct, I was unaware of your PR, next time you could attach it to the issue to make it more apparent :D
I have a follow-up question to you: should all the defined --oidc-extra-scope be passed along with the openid scope in the scope argument of the UserManager constructor?

Copy link
Contributor Author

@chriskari chriskari Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

meaning if you have defined --oidc-extra-scope=scope_a, --oidc-extra-scope=scope_b should the scope passed to the oidc UserManager be scope: "openid scope_a scope_b"?

Copy link
Contributor

@v0lkc v0lkc Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I thought it would be enough to link the fix to the issue. Next time I'll put it right at the beginning of the issue description. Regarding your question: you're right, the scopes have to be passed like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I will incorporate your changes in my PR 👍

setAuth(null);
const contextName = kubeconfig['current-context'];
if (!kubeconfig.contexts?.length) {
Expand Down Expand Up @@ -166,7 +173,7 @@ export function AddClusterWizard({ kubeconfig, setKubeconfig, config }) {
firstStep={true}
onCancel={() => setShowWizard(false)}
validation={!kubeconfig}
className="cluster-wizard__buttons"
className="cluster-wizard__buttons__sticky"
/>
</WizardStep>

Expand Down Expand Up @@ -197,7 +204,7 @@ export function AddClusterWizard({ kubeconfig, setKubeconfig, config }) {
setSelected={setSelected}
onCancel={() => setShowWizard(false)}
validation={!authValid}
className="cluster-wizard__buttons"
className="cluster-wizard__buttons__absolute"
/>
</WizardStep>
)}
Expand All @@ -217,14 +224,14 @@ export function AddClusterWizard({ kubeconfig, setKubeconfig, config }) {
data-step={!hasAuth || !hasOneContext ? '3' : '2'}
>
<div className="add-cluster__content-container">
<Title level="H5">
<Title level="H5" style={spacing.sapUiSmallMarginBottom}>
{t('clusters.storage.choose-storage.label')}
<>
<Button
id="storageDescriptionOpener"
icon="hint"
design="Transparent"
style={spacing.sapUiTinyMargin}
style={spacing.sapUiTinyMarginBegin}
onClick={() => setShowTitleDescription(true)}
/>
{createPortal(
Expand All @@ -248,7 +255,7 @@ export function AddClusterWizard({ kubeconfig, setKubeconfig, config }) {
setSelected={setSelected}
validation={!storage}
onCancel={() => setShowWizard(false)}
className="cluster-wizard__buttons"
className="cluster-wizard__buttons__absolute"
/>
</div>
</WizardStep>
Expand Down Expand Up @@ -284,7 +291,7 @@ export function AddClusterWizard({ kubeconfig, setKubeconfig, config }) {
setIsFormOpen({ formOpen: false });
}}
validation={!storage}
className="cluster-wizard__buttons"
className="cluster-wizard__buttons__sticky"
/>
</WizardStep>
</Wizard>
Expand Down
37 changes: 27 additions & 10 deletions src/components/Clusters/components/AddClusterWizard.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
.add-cluster-wizard-dialog::part(content) {
height: 90vh;
}

.add-cluster {
&__content-container {
background-color: var(--sapGroup_TitleBackground);
Expand All @@ -11,20 +15,17 @@
}

ui5-wizard {
height: 90vh;
min-width: 80vw;
height: 100%;
width: 75vw;

.cluster-wizard {
&__buttons {
position: absolute;
@mixin cluster-wizard-buttons-style {
z-index: 100;
bottom: 1rem;
left: 0;
display: flex;
justify-content: flex-end;
box-sizing: border-box;
width: calc(100% - 20px);
margin: 0 10px;
padding: 7px;
padding: 0.5rem;
border: 1px solid var(--sapGroup_TitleBorderColor);
border-radius: var(--sapElement_BorderCornerRadius);
background-color: var(--sapGroup_TitleBackground);
Expand All @@ -34,6 +35,23 @@ ui5-wizard {
color-mix(in srgb, var(--sapContent_ShadowColor) 16%, transparent);
}

&__buttons {
&__sticky {
position: sticky;
width: calc(100% + 32px);
margin: 0 -16px;
@include cluster-wizard-buttons-style;
}

&__absolute {
position: absolute;
left: 0;
width: calc(100% - 32px);
margin: 0 16px;
@include cluster-wizard-buttons-style;
}
}

&__token-info,
&__storage-preference {
color: var(--sapContent_LabelColor);
Expand All @@ -44,7 +62,6 @@ ui5-wizard {
display: flex;
flex-flow: column;
overflow: auto;
height: calc(90vh - 8rem);

.cluster-wizard__auth-form {
height: unset;
Expand All @@ -53,7 +70,7 @@ ui5-wizard {
}

.resource-form {
padding: 1rem 0;
padding: 0;

.form-field {
justify-content: start;
Expand Down
19 changes: 10 additions & 9 deletions src/components/Clusters/components/AuthForm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,7 @@ import * as Inputs from 'shared/ResourceForm/inputs';
import { getUser, getUserIndex } from '../shared';

import { spacing } from '@ui5/webcomponents-react-base';

export const AUTH_FORM_TOKEN = 'Token';
export const AUTH_FORM_OIDC = 'OIDC';
export const DEFAULT_SCOPE_VALUE = 'openid';
import { TextArrayInput } from 'shared/ResourceForm/fields';

const OIDCform = ({ resource, setResource, ...props }) => {
const { t } = useTranslation();
Expand Down Expand Up @@ -57,11 +54,11 @@ const OIDCform = ({ resource, setResource, ...props }) => {
input={Inputs.Text}
aria-label="client-secret"
/>
<ResourceForm.FormField
<TextArrayInput
required
propertyPath="$.scope"
label={t('clusters.wizard.auth.scopes')}
input={Inputs.Text}
defaultOpen
propertyPath="$.scopes"
title={t('clusters.wizard.auth.scopes')}
aria-label="scopes"
/>
</ResourceForm.Wrapper>
Expand Down Expand Up @@ -147,7 +144,11 @@ export function AuthForm({
<ResourceForm.FormField
label={t('clusters.wizard.auth.using-oidc')}
input={() => (
<Switch checked={useOidc} onChange={switchAuthVariant} />
<Switch
style={spacing.sapUiTinyMarginTop}
checked={useOidc}
onChange={switchAuthVariant}
/>
)}
className="oidc-switch"
/>
Expand Down
6 changes: 5 additions & 1 deletion src/components/Clusters/components/ChooseStorage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
import { FlexBox, RadioButton } from '@ui5/webcomponents-react';
import { useTranslation } from 'react-i18next';
import { spacing } from '@ui5/webcomponents-react-base';
import './ChooseStorage.scss';

export function ChooseStorage({ storage, setStorage }) {
const { t } = useTranslation();

return (
<>
<p className="cluster-wizard__storage-preference">
<p
className="cluster-wizard__storage-preference"
style={spacing.sapUiTinyMarginBottom}
>
{`${t('clusters.storage.storage-preference')}:`}
</p>
<FlexBox direction="Column">
Expand Down
Loading
Loading