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

antgamdia · 2022-09-29T14:52:21Z

Description of the change

This PR is finally adding the changes related to the new JSON Schema-based table that aims to replace the old "basic form". This way, we can render a large amount of params with no lag and, what's more important: we get rid of the hacky ways to display a form using the form: true property in the schema.

I'll comment on some changes in-line to make the review process easier once the PR is ready.

Benefits

Kubeapps will be able to render any JSONSchema-compliant schema. The overall performance of the form view gets boosted as well.

Possible drawbacks

Custom components: we need to re-check with our adopters the current situation.

Applicable issues

Additionally,

closes Dashboard/React/BasicDeploymentForm: Number precision issues in SliderParam if sliderStep is set to a floating number #5114 (as we are just using native HTML step in both the slider and the input here)
closes Dashboard/React: "restore defaults" button does not reset BasicDeploymentForm input fields #5025 (as the restore button now works as expected)
closes Migrate Basic Deployment Form to Clarity Core Components #2540 (as we have used CDS components in this new approach)
closes Feature Request: Required form values #1924 (as the required prop is now being added if present in the schema)
closes Helm: Kubeapps doesn't consider the default values set from the chart #4803 (as the issue seems to be gone in this PR)

Additional information

Still a draft as some tests are still pending.

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

netlify · 2022-09-29T14:52:26Z

✅ Deploy Preview for kubeapps-dev ready!

Name	Link
🔨 Latest commit	`f48ec48`
🔍 Latest deploy log	https://app.netlify.com/sites/kubeapps-dev/deploys/633d64dea71cdf0009b75ae8
😎 Deploy Preview	https://deploy-preview-5412--kubeapps-dev.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

github-actions

license-eye has totally checked 1414 files.

Valid	Invalid	Ignored	Fixed
740	1	673	0

Click to see the invalid file list

dashboard/src/components/DeploymentForm/DeploymentFormBody/BasicDeploymentForm/TabularSchemaEditorTable/TabularSchemaEditorTableRenderer.tsx

...ntFormBody/BasicDeploymentForm/TabularSchemaEditorTable/TabularSchemaEditorTableRenderer.tsx

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia · 2022-10-03T11:39:24Z

CI passed 🎉 Marking as ready for review + adding in-line comments to the code.

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia · 2022-10-03T11:39:48Z

dashboard/package.json

+    "react-dom": "^17.0.2",
    "react-scripts": "^5.0.1",
+    "react-test-renderer": "^17.0.2",


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

antgamdia · 2022-10-03T11:40:48Z

dashboard/src/components/DeploymentForm/DeploymentForm.tsx

@@ -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}>


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.

antgamdia · 2022-10-03T11:42:23Z

...mponents/DeploymentForm/DeploymentFormBody/AdvancedDeploymentForm/AdvancedDeploymentForm.tsx

+    valuesFromTheParentContainer,
+  ]);
+
+  const editorDidMount = (editor: monaco.editor.IStandaloneDiffEditor, m: typeof monaco) => {


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

antgamdia · 2022-10-03T11:47:20Z

...ymentForm/DeploymentFormBody/BasicDeploymentForm/TabularSchemaEditorTable/DebouncedInput.tsx

+import { CdsInput } from "@cds/react/input";
+import { InputHTMLAttributes, useEffect, useState } from "react";
+
+export default function DebouncedInput({


This component was mainly extracted from the Table documentation; in the future, we can think of reusing the same for other inputs (like the catalog search)

antgamdia · 2022-10-03T11:57:42Z

...ntForm/DeploymentFormBody/BasicDeploymentForm/TabularSchemaEditorTable/Params/ArrayParam.tsx

+    }
+  };
+
+  const onAddArrayItem = () => {


Init the array value with some value when clicking on "+"

antgamdia · 2022-10-03T12:12:53Z

dashboard/src/components/DeploymentForm/DeploymentFormBody/DeploymentFormBody.tsx

+    {} as YAML.Document.Parsed<YAML.ParsedNode>,
+  );
+  const [restoreModalIsOpen, setRestoreModalOpen] = useState(false);
+  const [isLoaded, setIsloaded] = useState(false);


Handles the very first component render

antgamdia · 2022-10-03T12:13:43Z

dashboard/src/components/DeploymentForm/DeploymentFormBody/DeploymentFormBody.tsx

+  );
+  const [restoreModalIsOpen, setRestoreModalOpen] = useState(false);
+  const [isLoaded, setIsloaded] = useState(false);
+  const [isLoading, setIsloading] = useState(true);


Handles each loading time when the YAML is being parsed back into nodes (eg, when the text editor is changed)

antgamdia · 2022-10-03T12:21:55Z

dashboard/src/shared/types.ts

-  customComponent?: object;
+// We extend the JSONSchema properties to include the default/deployed values as well as
+// other useful information for rendering each param in the UI
+export type IBasicFormParam = JSONSchemaType<any> & {


This way, we simply extend the existing JSONSchemaType with some operational field we are gonna need for rendering the table easily.

antgamdia · 2022-10-03T12:24:48Z

dashboard/src/shared/schema.ts


-// retrieveBasicFormParams iterates over a JSON Schema properties looking for `form` keys
-// It uses the raw yaml to setup default values.
-// It returns a key:value map for easier handling.
 export function retrieveBasicFormParams(


This is the core function for, given a jsonschema + values file, creating the array of nested params. The logic is simple: if this is a property without children, extract everything valuable and add it to the results. Otherwise, continue exploring.

antgamdia · 2022-10-03T12:25:40Z

dashboard/src/shared/yamlUtils.ts

@@ -0,0 +1,128 @@
+// Copyright 2019-2022 the Kubeapps contributors.
+// SPDX-License-Identifier: Apache-2.0
+


I've extracted plenty of the logic formerly present on the "schema.ts" file here, just to separate their concerns.

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia · 2022-10-03T17:35:44Z

dashboard/src/actions/installedpackages.ts

+        const schemaValidation = validateSchema(schema);
+        if (!schemaValidation.valid) {
+          const errorText = schemaValidation?.errors
+            ?.map(e => `  - ${e.instancePath}: ${e.message}`)
+            .join("\n");
+          throw new UnprocessableEntityError(
+            `The schema for this package is not valid. Please contact the package author. The following errors were found:\n${errorText}`,
+          );
+        }


Validating the schema before actually creating the InstalledPackage. I have considered adding a "force button", however, Helm is also failing if a wrong schema is added (as it is validating against the chart.values.schema.json if present in the tar.gz). A "force" button would imply some extra backend work.

Currently, for us, the problem is mainly just bitnami/readme-generator-for-helm#34.

antgamdia · 2022-10-03T17:37:10Z

...ntForm/DeploymentFormBody/BasicDeploymentForm/TabularSchemaEditorTable/Params/ArrayParam.tsx

+          <>
+            <CdsInput className="self-center">
+              <input
+                required={param.required}


Adding this basic HTML-based validation for solving this issue: #1924

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

castelblanque · 2022-10-05T09:33:45Z

dashboard/src/components/DeploymentForm/DeploymentForm.tsx

@@ -267,6 +267,7 @@ export default function DeploymentForm() {
                setValues={handleValuesChange}
                appValues={appValues}
                setValuesModified={setValuesModifiedTrue}
+                formRef={formRef}


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?

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.

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

castelblanque · 2022-10-05T10:30:53Z

dashboard/src/shared/types.ts

+  isCustomComponent?: boolean;
+};
+
+export interface IAjvValidateResult {


Why using this "cryptic"" name? 😅

No strong opinion on the naming, but since it is the result of performing the validate function on anajv (the library for handling schemas) object, this name is the best I could think of. Suggestions? :P

Just mentioned that because reading it for the first time sounds difficult to identify what is the purpose.
Not important, though. What about ISchemaValidationResult or similar? all in all, the types used inside the interface are not from ajv. If we switch to another library in the future, one less thing we need to touch.

Well, ErrorObject, one of the used types, is indeed imported from the Ajv library, so we would be kinda coupled to ajv anyway? But yep, at least we should add a comment explaining what is this interface for.

You are right. Fine leaving it as is with a comment.

castelblanque · 2022-10-05T10:47:58Z

integration/tests/main/07-upgrade.spec.js

-  await page.click('li:has-text("Changes")');
+
+  // Wait until changes are applied (due to the debounce in the input)
+  await page.waitForTimeout(1000);


Using waitForTimeout is always a potential source of problems.
When having problems or debugging, it gives a bit more information to use something like

const locator = page.locator("input[type='number']") await expect(locator).toHaveValue('2', {timeout: 1000})

BTW is there only one input of type number for this chart? Selector is very generic.

100% agree on the selector, will change it to be more specific.

wrt the waitForTimeout, I also agree, but I'm not sure the approach you mention worked in this case. Let me explain: once the user types something in an input, the actual HTML event will be only triggered after 500ms. So, unless you wait and stay in this very same component, the event won't get triggered. As a result, the diff view will always be empty, as there are no actual changes.
However, note that the <input ... locator will get the value 2 immediately, as the actual HTML value is not denounced, just the onChange event.

Therefore, I can't think of anything different but an actual active wait before continuing the rest of the interactions. In fact, I added two 1s-waits just for security, but a single one of 500ms right after the change should be enough.
Any suggestions? 😅

Ok, so it is the event that we are waiting here.
Could we add more info in the comment to specify that it is the event please?
Not sure if Playwright's waitForEvent + predicate func will help here.

Thanks for the tip, I've been exploring it, but unfortunately, the events it listens to are not HTML events, but some playwright-specific ones: backgroundpage, close, page, request, requestfailed, requestfinished, response, serviceworfker

In SO, some responses point to more sophisticated alternatives (ideally for more denounced inputs). Example: https://stackoverflow.com/questions/73739729/how-to-test-expected-delay-time-on-page-element-fixed-wait-debounce-with-play

So, for now, I think we should keep it simple, and leave the wait :(

castelblanque

Awesome work. Big step in making Kubeapps more generic and sustainable, thanks!

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia added 7 commits September 29, 2022 16:29

Remove unused deps

c2b71e8

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Refactor schema/yaml util functions

41b2ab9

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Delete no longer used components

bf0fd45

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Add some required changes in files

c23a1d6

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Add new json-schema tabular view

e73bffd

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Add basic form components

73e9fff

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Fix some tests

47866f2

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

vmwclabot added the cla-not-required label Sep 29, 2022

github-actions bot reviewed Sep 29, 2022

View reviewed changes

...ntFormBody/BasicDeploymentForm/TabularSchemaEditorTable/TabularSchemaEditorTableRenderer.tsx Show resolved Hide resolved

Fix linter issues

37e3a6f

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia force-pushed the 4396-table-basic-form branch from 492bcf2 to 37e3a6f Compare September 29, 2022 15:55

antgamdia added 10 commits September 29, 2022 18:14

Add test files (still WIP)

d63b338

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Fix e2e test

d46d171

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Merge branch 'main' into 4396-table-basic-form

d58e282

Fix booleanParam test

7bfb69d

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Fix textParam tests

2f5029d

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Fix sliderParam tests

ed6552a

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Add remaining unit tests

bde921a

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Fix e2e test

cd21b80

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Merge branch 'main' into 4396-table-basic-form

9655a5f

Add wait in e2e test to prevent deboucing

98d7fce

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia mentioned this pull request Oct 3, 2022

Adapt "basic forms" to a JSONSchema-compliant spec #4917

Closed

antgamdia added 2 commits October 3, 2022 14:19

remove leftover

486cc47

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

remove unused useEffect

ebf442c

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia commented Oct 3, 2022

View reviewed changes

antgamdia marked this pull request as ready for review October 3, 2022 12:29

antgamdia added 2 commits October 3, 2022 16:43

move leftover file

57dfb67

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

add simple validation

7b12f70

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia added 2 commits October 3, 2022 17:25

Fix issue with duplicate key indexes

b698179

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Validate schema before installing/upgrading

c6462b8

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia commented Oct 3, 2022

View reviewed changes

fix linter issue

035172a

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

antgamdia requested a review from castelblanque October 4, 2022 12:30

Remove max/min in the input of the SliderParam

07bfec2

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

castelblanque reviewed Oct 5, 2022

View reviewed changes

castelblanque approved these changes Oct 5, 2022

View reviewed changes

antgamdia added 3 commits October 5, 2022 12:52

add comments in some types

f919322

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Make replicas selector more specific in e2e test

a5fb3b2

Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>

Merge branch 'main' into 4396-table-basic-form

f48ec48

antgamdia merged commit 19a72e3 into main Oct 5, 2022

antgamdia deleted the 4396-table-basic-form branch October 5, 2022 12:03

This was referenced Oct 5, 2022

JSONSchema table - fixes #5432

Merged

Check if both js-yaml and YAML libraries are still required #5435

Closed

Create additional "JSON Schema form" param renderers #5436

Closed

Allow searching params in nested values of the JSON Schema table #5437

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

antgamdia commented Sep 29, 2022 •

edited

netlify bot commented Sep 29, 2022 •

edited

github-actions bot left a comment

antgamdia commented Oct 3, 2022

antgamdia Oct 3, 2022

antgamdia Oct 3, 2022

antgamdia Oct 3, 2022

antgamdia Oct 3, 2022

antgamdia Oct 3, 2022

antgamdia Oct 3, 2022

antgamdia Oct 3, 2022

antgamdia Oct 3, 2022

antgamdia Oct 3, 2022

antgamdia Oct 3, 2022

antgamdia Oct 3, 2022

antgamdia Oct 3, 2022

castelblanque Oct 5, 2022

antgamdia Oct 5, 2022

antgamdia Oct 5, 2022

castelblanque Oct 5, 2022 •

edited

antgamdia Oct 5, 2022

castelblanque Oct 5, 2022

antgamdia Oct 5, 2022

castelblanque Oct 5, 2022

castelblanque Oct 5, 2022

antgamdia Oct 5, 2022

castelblanque Oct 5, 2022

antgamdia Oct 5, 2022

castelblanque left a comment

		@@ -0,0 +1,128 @@
		// Copyright 2019-2022 the Kubeapps contributors.
		// SPDX-License-Identifier: Apache-2.0

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

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

Conversation

antgamdia commented Sep 29, 2022 • edited

Description of the change

Benefits

Possible drawbacks

Applicable issues

Additional information

netlify bot commented Sep 29, 2022 • edited

✅ Deploy Preview for kubeapps-dev ready!

github-actions bot left a comment

Choose a reason for hiding this comment

antgamdia commented Oct 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

castelblanque Oct 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

castelblanque left a comment

Choose a reason for hiding this comment

antgamdia commented Sep 29, 2022 •

edited

netlify bot commented Sep 29, 2022 •

edited

castelblanque Oct 5, 2022 •

edited