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.

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

New deployment forms: add json-schema-based table #5412

Merged
merged 29 commits into from
Oct 5, 2022
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
c2b71e8
Remove unused deps
antgamdia Sep 29, 2022
41b2ab9
Refactor schema/yaml util functions
antgamdia Sep 29, 2022
bf0fd45
Delete no longer used components
antgamdia Sep 29, 2022
c23a1d6
Add some required changes in files
antgamdia Sep 29, 2022
e73bffd
Add new json-schema tabular view
antgamdia Sep 29, 2022
73e9fff
Add basic form components
antgamdia Sep 29, 2022
47866f2
Fix some tests
antgamdia Sep 29, 2022
37e3a6f
Fix linter issues
antgamdia Sep 29, 2022
d63b338
Add test files (still WIP)
antgamdia Sep 29, 2022
d46d171
Fix e2e test
antgamdia Sep 30, 2022
d58e282
Merge branch 'main' into 4396-table-basic-form
antgamdia Sep 30, 2022
7bfb69d
Fix booleanParam test
antgamdia Sep 30, 2022
2f5029d
Fix textParam tests
antgamdia Sep 30, 2022
ed6552a
Fix sliderParam tests
antgamdia Sep 30, 2022
bde921a
Add remaining unit tests
antgamdia Oct 1, 2022
cd21b80
Fix e2e test
antgamdia Oct 1, 2022
9655a5f
Merge branch 'main' into 4396-table-basic-form
antgamdia Oct 3, 2022
98d7fce
Add wait in e2e test to prevent deboucing
antgamdia Oct 3, 2022
486cc47
remove leftover
antgamdia Oct 3, 2022
ebf442c
remove unused useEffect
antgamdia Oct 3, 2022
57dfb67
move leftover file
antgamdia Oct 3, 2022
7b12f70
add simple validation
antgamdia Oct 3, 2022
b698179
Fix issue with duplicate key indexes
antgamdia Oct 3, 2022
c6462b8
Validate schema before installing/upgrading
antgamdia Oct 3, 2022
035172a
fix linter issue
antgamdia Oct 3, 2022
07bfec2
Remove max/min in the input of the SliderParam
antgamdia Oct 4, 2022
f919322
add comments in some types
antgamdia Oct 5, 2022
a5fb3b2
Make replicas selector more specific in e2e test
antgamdia Oct 5, 2022
f48ec48
Merge branch 'main' into 4396-table-basic-form
antgamdia Oct 5, 2022
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
7 changes: 2 additions & 5 deletions dashboard/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,7 @@
"protobufjs": "^7.1.2",
"qs": "^6.11.0",
"react": "^17.0.2",
"react-compound-slider": "^3.4.0",
"react-copy-to-clipboard": "^5.1.0",
"react-dom": "^17.0.2",
"react-helmet": "^6.1.0",
"react-intl": "^6.1.1",
"react-markdown": "^8.0.3",
Expand All @@ -61,9 +59,6 @@
"react-redux": "^7.2.9",
"react-router-dom": "^5.3.0",
"react-router-hash-link": "^2.4.3",
"react-switch": "^7.0.0",
"react-tabs": "^5.1.0",
"react-test-renderer": "^17.0.2",
"react-tooltip": "^4.2.21",
"react-transition-group": "^4.4.5",
"redux": "^4.2.0",
Expand Down Expand Up @@ -113,7 +108,9 @@
"postcss": "^8.4.16",
"postcss-scss": "^4.0.5",
"prettier": "^2.7.1",
"react-dom": "^17.0.2",
"react-scripts": "^5.0.1",
"react-test-renderer": "^17.0.2",
Comment on lines +111 to +113
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to dev-deps as they are not required in the production build

"redux-mock-store": "^1.5.4",
"sass": "^1.55.0",
"shx": "^0.3.4",
Expand Down
6 changes: 3 additions & 3 deletions dashboard/src/actions/installedpackages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
import { getPluginsSupportingRollback } from "shared/utils";
import { ActionType, deprecated } from "typesafe-actions";
import { InstalledPackage } from "../shared/InstalledPackage";
import { validate } from "../shared/schema";
import { validateValuesSchema } from "../shared/schema";
import { handleErrorAction } from "./auth";

const { createAction } = deprecated;
Expand Down Expand Up @@ -234,7 +234,7 @@ export function installPackage(
dispatch(requestInstallPackage());
try {
if (values && schema) {
const validation = validate(values, schema);
const validation = validateValuesSchema(values, schema);
if (!validation.valid) {
const errorText =
validation.errors &&
Expand Down Expand Up @@ -283,7 +283,7 @@ export function updateInstalledPackage(
dispatch(requestUpdateInstalledPackage());
try {
if (values && schema) {
const validation = validate(values, schema);
const validation = validateValuesSchema(values, schema);
if (!validation.valid) {
const errorText =
validation.errors &&
Expand Down
4 changes: 2 additions & 2 deletions dashboard/src/components/AppUpgrade/AppUpgrade.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ beforeEach(() => {
})),
});

// mock the window.ResizeObserver, required by the MonacoEditor for the layout
// mock the window.ResizeObserver, required by the MonacoDiffEditor for the layout
Object.defineProperty(window, "ResizeObserver", {
writable: true,
configurable: true,
Expand All @@ -142,7 +142,7 @@ beforeEach(() => {
})),
});

// mock the window.HTMLCanvasElement.getContext(), required by the MonacoEditor for the layout
// mock the window.HTMLCanvasElement.getContext(), required by the MonacoDiffEditor for the layout
Object.defineProperty(HTMLCanvasElement.prototype, "getContext", {
writable: true,
configurable: true,
Expand Down
1 change: 1 addition & 0 deletions dashboard/src/components/AppView/AppView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,7 @@ export default function AppView() {
<div className="appview-separator">
<AppValues
values={
//TODO(agamez): check if using this yaml.dump/load is really needed
selectedInstalledPkg?.valuesApplied
? yaml.dump(yaml.load(selectedInstalledPkg.valuesApplied))
: ""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ import { MemoryRouter, Route, Router } from "react-router-dom";
import { Kube } from "shared/Kube";
import { getStore, mountWrapper } from "shared/specs/mountWrapper";
import { FetchError, IStoreState, PluginNames } from "shared/types";
import DeploymentFormBody from "../DeploymentFormBody/DeploymentFormBody";
import DeploymentForm from "./DeploymentForm";
import DeploymentFormBody from "./DeploymentFormBody";

const defaultProps = {
pkgName: "foo",
Expand Down Expand Up @@ -79,7 +79,7 @@ beforeEach(() => {
})),
});

// mock the window.ResizeObserver, required by the MonacoEditor for the layout
// mock the window.ResizeObserver, required by the MonacoDiffEditor for the layout
Object.defineProperty(window, "ResizeObserver", {
writable: true,
configurable: true,
Expand All @@ -90,7 +90,7 @@ beforeEach(() => {
})),
});

// mock the window.HTMLCanvasElement.getContext(), required by the MonacoEditor for the layout
// mock the window.HTMLCanvasElement.getContext(), required by the MonacoDiffEditor for the layout
Object.defineProperty(HTMLCanvasElement.prototype, "getContext", {
writable: true,
configurable: true,
Expand Down
11 changes: 6 additions & 5 deletions dashboard/src/components/DeploymentForm/DeploymentForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,25 +10,24 @@ import AvailablePackageDetailExcerpt from "components/Catalog/AvailablePackageDe
import Alert from "components/js/Alert";
import Column from "components/js/Column";
import Row from "components/js/Row";
import LoadingWrapper from "components/LoadingWrapper";
import PackageHeader from "components/PackageHeader/PackageHeader";
import { push } from "connected-react-router";
import {
AvailablePackageReference,
ReconciliationOptions,
} from "gen/kubeappsapis/core/packages/v1alpha1/packages";
import { Plugin } from "gen/kubeappsapis/core/plugins/v1alpha1/plugins";
import { useEffect, useState } from "react";
import { useEffect, useRef, useState } from "react";
import { useDispatch, useSelector } from "react-redux";
import * as ReactRouter from "react-router-dom";
import "react-tabs/style/react-tabs.css";
import { Action } from "redux";
import { ThunkDispatch } from "redux-thunk";
import { Kube } from "shared/Kube";
import { FetchError, IStoreState } from "shared/types";
import * as url from "shared/url";
import { getPluginsRequiringSA, k8sObjectNameRegex } from "shared/utils";
import DeploymentFormBody from "../DeploymentFormBody/DeploymentFormBody";
import LoadingWrapper from "../LoadingWrapper/LoadingWrapper";
import DeploymentFormBody from "./DeploymentFormBody";
interface IRouteParams {
cluster: string;
namespace: string;
Expand Down Expand Up @@ -63,6 +62,7 @@ export default function DeploymentForm() {
const [valuesModified, setValuesModified] = useState(false);
const [serviceAccountList, setServiceAccountList] = useState([] as string[]);
const [reconciliationOptions, setReconciliationOptions] = useState({} as ReconciliationOptions);
const formRef = useRef<HTMLFormElement>(null);

const error = apps.error || selectedPackage.error;

Expand Down Expand Up @@ -209,7 +209,7 @@ export default function DeploymentForm() {
</Column>
<Column span={9}>
{error && <Alert theme="danger">An error occurred: {error.message}</Alert>}
<form onSubmit={handleDeploy}>
<form onSubmit={handleDeploy} ref={formRef}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a hacky way to pass information from the children to the parent container in React. We need to manually control the form submission downstream.

<CdsFormGroup
validate={true}
className="deployment-form"
Expand Down Expand Up @@ -267,6 +267,7 @@ export default function DeploymentForm() {
setValues={handleValuesChange}
appValues={appValues}
setValuesModified={setValuesModifiedTrue}
formRef={formRef}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that formRef is only used for performing a requestSubmit() in the parent.
Why not passing a plain callback function as parameter instead of this reference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was introduced in one of my previous attempts when working on this. The idea is to trigger a plain normal HTML form "submit" event from a child, this way we leverage the built-in HTML validations (like required, max, min, pattern).

Anyway, just to double-check, I'm trying locally to replace the ref with the handleDeploy() function directly to ensure the ref is really needed or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, that's why I left the hacky ref, otherwise the HTML are not taken into account :(

nohtmlval

/>
</form>
</Column>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,233 @@
// Copyright 2019-2022 the Kubeapps contributors.
// SPDX-License-Identifier: Apache-2.0

import { CdsRadio, CdsRadioGroup } from "@cds/react/radio";
import Column from "components/js/Column";
import Row from "components/js/Row";
import monaco from "monaco-editor/esm/vs/editor/editor.api"; // for types only
import { useEffect, useState } from "react";
import { MonacoDiffEditor } from "react-monaco-editor";
import { useSelector } from "react-redux";
import { IStoreState } from "shared/types";

export interface IAdvancedDeploymentForm {
valuesFromTheParentContainer?: string;
handleValuesChange: (value: string) => void;
children?: JSX.Element;
valuesFromTheDeployedPackage: string;
valuesFromTheAvailablePackage: string;
deploymentEvent: string;
}

export default function AdvancedDeploymentForm(props: IAdvancedDeploymentForm) {
const {
config: { theme },
} = useSelector((state: IStoreState) => state);
const {
handleValuesChange,
valuesFromTheParentContainer,
valuesFromTheDeployedPackage,
valuesFromTheAvailablePackage,
deploymentEvent,
} = props;

const [usePackageDefaults, setUsePackageDefaults] = useState(
deploymentEvent === "upgrade" ? false : true,
);
const [useDiffEditor, setUseDiffEditor] = useState(true);
const [diffValues, setDiffValues] = useState(valuesFromTheAvailablePackage);

const diffEditorOptions = {
renderSideBySide: false,
automaticLayout: true,
};

const onChange = (value: string | undefined, _ev: any) => {
// debouncing is not required as the diff calculation happens in a webworker
handleValuesChange(value || "");
};

useEffect(() => {
if (!useDiffEditor) {
setDiffValues(valuesFromTheParentContainer || "");
} else if (!usePackageDefaults) {
setDiffValues(valuesFromTheDeployedPackage);
} else {
setDiffValues(valuesFromTheAvailablePackage);
}
}, [
usePackageDefaults,
useDiffEditor,
valuesFromTheAvailablePackage,
valuesFromTheDeployedPackage,
valuesFromTheParentContainer,
]);

const editorDidMount = (editor: monaco.editor.IStandaloneDiffEditor, m: typeof monaco) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we are just creating some commands in the palette and context menu (when you right-click on the editor).

// Add "go to the next change" action
editor.addAction({
id: "goToNextChange",
label: "Go to the next change",
keybindings: [m.KeyMod.Alt | m.KeyCode.KeyG],
contextMenuGroupId: "9_cutcopypaste",
run: () => {
const lineChanges = editor?.getLineChanges() as monaco.editor.ILineChange[];
lineChanges.some(lineChange => {
const currentPosition = editor?.getPosition() as monaco.Position;
if (currentPosition.lineNumber < lineChange.modifiedEndLineNumber) {
// Set the cursor to the next change
editor?.setPosition({
lineNumber: lineChange.modifiedEndLineNumber,
column: 1,
});
// Scroll to the next change
editor?.revealPositionInCenter({
lineNumber: lineChange.modifiedEndLineNumber,
column: 1,
});
// Return true to stop the loop
return true;
}
return false;
});
},
});
// Add "go to the previous change" action
editor.addAction({
id: "goToPreviousChange",
label: "Go to the previous change",
keybindings: [m.KeyMod.Alt | m.KeyCode.KeyF],
contextMenuGroupId: "9_cutcopypaste",
run: () => {
const lineChanges = editor?.getLineChanges() as monaco.editor.ILineChange[];
lineChanges.some(lineChange => {
const currentPosition = editor?.getPosition() as monaco.Position;
if (currentPosition.lineNumber > lineChange.modifiedEndLineNumber) {
// Set the cursor to the next change
editor?.setPosition({
lineNumber: lineChange.modifiedEndLineNumber,
column: 1,
});
// Scroll to the next change
editor?.revealPositionInCenter({
lineNumber: lineChange.modifiedEndLineNumber,
column: 1,
});
// Return true to stop the loop
return true;
}
return false;
});
},
});

// Add the "toggle deployed/package default values" action
if (deploymentEvent === "upgrade") {
editor.addAction({
id: "useDefaultsFalse",
label: "Use default values",
keybindings: [m.KeyMod.Alt | m.KeyCode.KeyD],
contextMenuGroupId: "9_cutcopypaste",
run: () => {
setUsePackageDefaults(false);
},
});
editor.addAction({
id: "useDefaultsTrue",
label: "Use package values",
keybindings: [m.KeyMod.Alt | m.KeyCode.KeyV],
contextMenuGroupId: "9_cutcopypaste",
run: () => {
setUsePackageDefaults(true);
},
});
}
};

return (
<div className="deployment-form-tabs-data">
<>
<Row>
<Column span={3}>
<CdsRadioGroup layout="vertical">
{/* eslint-disable-next-line jsx-a11y/label-has-associated-control */}
<label>Enable diff editor:</label>
<CdsRadio>
<label htmlFor="diff-compare-enable-true">Yes</label>
<input
id="diff-compare-enable-true"
type="radio"
name="true"
checked={useDiffEditor}
onChange={e => {
setUseDiffEditor(e.target.checked);
}}
/>
</CdsRadio>
<CdsRadio>
<label htmlFor="diff-compare-enable-false">No</label>
<input
id="diff-compare-enable-false"
type="radio"
name="deployed"
checked={!useDiffEditor}
onChange={e => {
setUseDiffEditor(!e.target.checked);
}}
/>
</CdsRadio>
</CdsRadioGroup>
</Column>
{deploymentEvent === "upgrade" ? (
<>
<Column span={3}>
<CdsRadioGroup layout="vertical">
{/* eslint-disable-next-line jsx-a11y/label-has-associated-control */}
<label>Values to compare against:</label>
<CdsRadio>
<label htmlFor="diff-compare-values-package">Package values</label>
<input
id="diff-compare-values-package"
type="radio"
name="package"
checked={usePackageDefaults}
onChange={e => {
setUsePackageDefaults(e.target.checked);
}}
/>
</CdsRadio>
<CdsRadio>
<label htmlFor="diff-compare-values-deployed">Deployed values</label>
<input
id="diff-compare-values-deployed"
type="radio"
name="deployed"
checked={!usePackageDefaults}
onChange={e => {
setUsePackageDefaults(!e.target.checked);
}}
/>
</CdsRadio>
</CdsRadioGroup>
</Column>
</>
) : (
<></>
)}
</Row>
</>
<br />
<MonacoDiffEditor
value={valuesFromTheParentContainer}
original={diffValues}
className="editor"
height="90vh"
language="yaml"
theme={theme === "dark" ? "vs-dark" : "light"}
options={diffEditorOptions}
onChange={onChange}
editorDidMount={editorDidMount}
/>
</div>
);
}
Loading