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

Component for editing package installation values in table mode #4396

Closed
castelblanque opened this issue Mar 8, 2022 · 14 comments · Fixed by #5412
Closed

Component for editing package installation values in table mode #4396

castelblanque opened this issue Mar 8, 2022 · 14 comments · Fixed by #5412
Assignees
Labels
component/ui Issue related to kubeapps UI kind/feature An issue that reports a feature (approved) to be implemented
Projects

Comments

@castelblanque
Copy link
Collaborator

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.)

@project-bot project-bot bot added this to Inbox in Kubeapps Mar 8, 2022
@castelblanque castelblanque added kind/feature An issue that reports a feature (approved) to be implemented component/ui Issue related to kubeapps UI labels Mar 8, 2022
@antgamdia antgamdia added this to the Form refactor milestone Mar 8, 2022
@absoludity
Copy link
Contributor

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.

@ppbaena ppbaena moved this from Inbox to Backlog in Kubeapps Mar 15, 2022
@ppbaena ppbaena added kind/enhancement An issue that reports an enhancement for an implemented feature kind/proposal An issue that reports a new feature proposal to be discussed and removed kind/feature An issue that reports a feature (approved) to be implemented kind/enhancement An issue that reports an enhancement for an implemented feature labels Jun 8, 2022
@antgamdia
Copy link
Contributor

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

@castelblanque
Copy link
Collaborator Author

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?

@antgamdia antgamdia self-assigned this Aug 29, 2022
@antgamdia
Copy link
Contributor

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

Image

@castelblanque
Copy link
Collaborator Author

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

@antgamdia
Copy link
Contributor

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
Copy link
Contributor

More progress:

Image

@antgamdia
Copy link
Contributor

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

@castelblanque
Copy link
Collaborator Author

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?

@ppbaena
Copy link
Collaborator

ppbaena commented Sep 15, 2022

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

@antgamdia
Copy link
Contributor

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.

@castelblanque
Copy link
Collaborator Author

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?

@antgamdia
Copy link
Contributor

antgamdia commented Oct 3, 2022

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
Copy link
Contributor

antgamdia commented Oct 3, 2022

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: #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: bitnami/readme-generator-for-helm#34

The other issue is fortunately gone now:

issueGone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/ui Issue related to kubeapps UI kind/feature An issue that reports a feature (approved) to be implemented
Projects
Archived in project
Kubeapps
  
Backlog
Development

Successfully merging a pull request may close this issue.

4 participants