kubeapps icon indicating copy to clipboard operation
kubeapps copied to clipboard

Component for editing package installation values in table mode

Open castelblanque opened this issue 2 years ago • 3 comments

Description:

Currently Kubeapps is capable of dynamically generating a form with chart values metadata so that users can use it to adjust installation values. Sometimes this kind of dynamic forms are a bit sluggish, specially when the form is very large. Also, finding the right control that corresponds to the right installation value is sometimes complicated.

It could be nice being able to edit installation values in table mode. One row -> one installation value. Something like TMC colleagues have done:

Key Type Description Value
global Global configuration
imageimageRegistry string Global image registry docker.io/...
imageimagePullSecrets array Secrets.... ["secret1"] *
commonAnnotations object Annotations common to all resources {}
startupProbe
imagetimeoutSeconds integer Timeout seconds for startup probe 20 *

Information presented on screen would be the same, but smaller in size and quicker to edit than the form edition. It would be straight forward to know the corresponding package value key. The only editable part would be the Value column. Edited values would have some kind of visual sign (e.g. asterisk, different color, etc.)

castelblanque avatar Mar 08 '22 13:03 castelblanque

This might also make it simpler for us to have a standard way to render forms both for simple form components as well as schema items for extra fields required by packaging plugins.

absoludity avatar Mar 08 '22 23:03 absoludity

More updates here:

  • As the schema increases, dealing with a real-time-in-yaml-param-setting process is unbearable. I have switched to a new model where we store the modifications and then apply them altogether as user's request.
  • Unfortunately, this is still not solving the rendering time when the schema is long. I think I'll go with an external Table solution like https://tanstack.com/table. However, I wanted to have something working before going deeper into the table think.
  • Some good news: even being in the table, I think we can reuse some logic for setting the , and other components to ease the UX when entering data.
  • I haven't thought yet of the "custom forms" feature. I think we have to preserve this functionality as it is being used by our community. However, this is for a 2nd stage.

newParams

antgamdia avatar Aug 26 '22 17:08 antgamdia

dealing with a real-time-in-yaml-param-setting process is unbearable. I have switched to a new model where we store the modifications and then apply them altogether as user's request.

Yep, too much stuff for browser's JS. Probably the best way to go is to stack the changes and apply them as you did.

apply them altogether as user's request.

I don't think we should put on the user's decision the result of a technical limitation (i.e. remembering to click "sync"). Is it sluggish always for all Yaml sizes? Could we behave differently in two scenarios (big and small yaml files?). I'm a bit reluctant about having that "Sync" button, wondering what will happen when:

  1. No "Live edit"
  2. User changes a param in "Form" tab
  3. Forgets to click "Sync"
  4. Goes to "Changes" tab (no param updated)
  5. Clicks "Deploy" User will see the default value (not synced in "Changes"), but the one from "Form" will be applied?

What do you think about removing the sync button, and automatically doing the actual sync in both?:

  • click any other tab
  • click "Deploy"

We could also try to leverage Web workers?

castelblanque avatar Aug 29 '22 06:08 castelblanque

Just sharing the current progress wrt the new table, will reply to the comment soon.

Image

antgamdia avatar Sep 09 '22 17:09 antgamdia

It is cool that there is a search option, great progress! Did you discard the foldable sections (maybe lazy-loading)?

castelblanque avatar Sep 12 '22 07:09 castelblanque

I don't think we should put on the user's decision the result of a technical limitation (i.e. remembering to click "sync").

Neither do I: it was implemented like a manual action but just while developing. The underlying action is expected to be triggered automatically, for instance, always deferring the sync process to the moment when the user changes the tab.

Is it sluggish always for all Yaml sizes? Could we behave differently in two scenarios (big and small yaml files?).

I have tested the "old" custom from (aka the bitnami one, just using a reduced subset of params) and it works perfectly. However, is when using the full schema (at least in the bitnami charts, as they are pretty configurable) that the lag appears.

I'm a bit reluctant about having that "Sync" button, wondering what will happen when:

No "Live edit" User changes a param in "Form" tab Forgets to click "Sync" Goes to "Changes" tab (no param updated) Clicks "Deploy" User will see the default value (not synced in "Changes"), but the one from "Form" will be applied?

Technically, it will appear a browser-native pop-up saying that you have unsaved changes. I was playing around with the beforeunload events. That said, I think we should try to avoid this use case.

What do you think about removing the sync button, and automatically doing the actual sync in both?

100% agree

We could also try to leverage Web workers?

Good idea, not sure how can we use them along with the usual react's state typical management, but, for sure it is an interesting option to run such a lengthy operation like this one in bg.

Did you discard the foldable sections (maybe lazy-loading)?

Not at all! I just prioritized having a "working" table and just added a POC of the filters and pagination. Collapsible rows is something I wanna explore shortly.

antgamdia avatar Sep 12 '22 09:09 antgamdia

More progress:

Image

antgamdia avatar Sep 14 '22 08:09 antgamdia

Today's update:

After implementing the groping by parent property, I've sketched up the sync flow inside the component and "upstream" (to the parent's). I've tried to minimize the YAML parsing operations for perfornce and, instead, the table parms are updated locally.

The observed response times seem to be much better than before, so perhaps storing the modifications and applying them in batch when switching tabs is not that necessary. Reason? Now we're dealing with a nested array instead of a flattened one with hundreds of params.

Pending stuff from the top of my mind: 1) use the "deployed value", "default value from the schema", "default value from the values" properly; 2) improve the performance of the yaml editor; 3) improve the performance of the initial load (it takes several seconds)

Image

antgamdia avatar Sep 14 '22 17:09 antgamdia

Awesome, that looks always better!!

Some minor observations:

  • Columnns change width when searching or paginating, probably adapting to the content. Shouldn't we use fixed size columns so that it looks more uniform?
  • Clarity uses carets for marking folded/unfolded sections, e.g. Tree views.
  • Have you considered using Clarity's stack view with lazy loading for children? Not sure if it is available for React, though.
  • I guess "Deployed value" column shouldn't be visible for new deployments?

castelblanque avatar Sep 15 '22 07:09 castelblanque

Awesome work @antgamdia !!! Looking forward for this table view available in Kubeapps 🚀

ppbaena avatar Sep 15 '22 07:09 ppbaena

Sharing the progress with some gifs, but will describe the approach + reply to the comments soon. For now the quick summary is: I've removed the old ace text editor + react-diff component in favor of a single instance of a Monaco (aka vs code engine) diff editor. Since it uses webworkers under the hood, the overall performance is way better.

There are plenty of bugs and open issues in the current design, especially, some glitches when editing, since more deboucing is still required.

Current performance when using the biggest bitnami chart's values (thanos): newTable4Perf

The editions in the table + diffEditor (+ real time feedback about what changed): newTable4Editor

The diff view now allows choosing between diffing the deployed values or the package values (eg. a new version): newTable4Upgr

PS: I've been trying to add https://monaco-yaml.js.org/ (note the actual YAML validation against a json schema; it's awesome), but after some time dealing with webpack, I gave up (for now) since I wanted to have a fully working scenario today.

antgamdia avatar Sep 16 '22 21:09 antgamdia

webworkers under the hood

Happy to see that webworkers work as expected. It is a really good job!

Minor observation about UI from the last GIF: the switch that toggles the diff mode confused me a bit. It wasn't clear what was the state when the toggle was off (due to being off), and then the label changes when switching. What do you think about replacing that control with two radio buttons or a single dropdown?

castelblanque avatar Sep 19 '22 07:09 castelblanque

I'll extract the TODOs into a separate issue, but, for the moment, let me quickly write them down here:

  • Check if both js-yaml and YAML libraries are still required
  • Check the custom basic form components feature with adopters:
    • For instance, render the custom components into a separate modal window.
  • Missing renderers:
    • 1st level: object (at least, as array of tuples <string, any> for defining each property.
      • Currently falling back to a textarea.
    • array level: enum, arrays and objects
      • Currently falling back to a textarea.
  • ~Add input validation based on the schema: there's some commented-out logic in the TextParam.tsx~ (done)
  • Allow searching params in nested values (currently just searching top-level params)

antgamdia avatar Oct 03 '22 09:10 antgamdia

When using a chart with a full schema (ie, for every value), I'm running into this issue:

image

which seems surprisingly similar to this other one: https://github.com/vmware-tanzu/kubeapps/issues/4803

I'm having a look at it.

EDIT: no, the problem is the schema itself indeed. In the case of empty arrays, the yielded schema is just wrong, wherefore the error: https://github.com/bitnami-labs/readme-generator-for-helm/issues/34

The other issue is fortunately gone now:

issueGone

antgamdia avatar Oct 03 '22 15:10 antgamdia