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

Add support for dynamic forms in helm install and upgrade #5790

Merged

Conversation

rohitkrai03
Copy link
Contributor

Fixes:

Depends on: #5789

Analysis / Root cause: We have YAML editor as default editor for values in Helm install and upgrade form. We need to support dynamic form generation if the helm chart has a value.schema.json present.

Solution Description: Once we have the custom formik fields to handle dynamic forms and toggle between form and yaml editor without loosing context, we need to use that to add support for synced editor in helm install and upgrade form.

Screen shots / Gifs for design review:
Helm Install -
Dynamic Forms Helm Install

Helm Upgrade -
Dynamic Forms Helm Upgrade

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci-robot openshift-ci-robot added the component/dev-console Related to dev-console label Jun 19, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 19, 2020
@rohitkrai03
Copy link
Contributor Author

/retest

@rohitkrai03
Copy link
Contributor Author

/hold

This PR depends on components being added in #5789.

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 19, 2020
@openshift-ci-robot openshift-ci-robot added component/olm Related to OLM component/shared Related to console-shared and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 23, 2020
@rohitkrai03
Copy link
Contributor Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 23, 2020
@rohitkrai03
Copy link
Contributor Author

/retest

1 similar comment
@rohitkrai03
Copy link
Contributor Author

/retest

@rohitkrai03 rohitkrai03 force-pushed the helm-dynamic-form branch 2 times, most recently from 6c21ed9 to 35c5f27 Compare June 23, 2020 16:56
@rohitkrai03
Copy link
Contributor Author

/retest

Copy link

@parvathyvr parvathyvr left a comment

Choose a reason for hiding this comment

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

@rohitkrai03 Since we have the Form and YAML view now,The message shown under 'Install Helm Chart' heading has changed.The message displayed changes based on the view shown.
Both the messages for Form view and YAML view can found in this High level design doc https://docs.google.com/document/d/1PCCE8rh1_URGVhaBYBYsGO9ghwwsGAec80XK53FJfvs/edit#heading=h.647eyup2doth
Also the message shown for Upgrade form will have to change.I will work on those messages and update in my final design document.Thanks!

@christianvogt
Copy link
Contributor

@rohitkrai03 is it possible to keep the expanded state of the form view when switching between form or yaml?

@rohitkrai03
Copy link
Contributor Author

@rohitkrai03 is it possible to keep the expanded state of the form view when switching between form or yaml?

@christianvogt Don't think it's possible to maintain that state, it's handled by DynamicForm component and the templates that it's using. I'll look into it and see if somehow we can do something.

@andrewballantyne
Copy link
Contributor

andrewballantyne commented Jun 24, 2020

@rohitkrai03 There may be something lost here... The "Root Schema" feels weird:

image

Would we not want to auto-expand the first level of the schema to avoid the collapsed block?

@rohitkrai03
Copy link
Contributor Author

rohitkrai03 commented Jun 24, 2020

@rohitkrai03 There may be something lost here... The "Root Schema" feels weird:

image

Would we not want to auto-expand the first level of the schema to avoid the collapsed block?

@andrewballantyne That actually depends on the schema provided by the chart. If any part of the schema has a title and description, the form will render it inside an ExpandCollapse. The current schema in the example Helm chart is not written properly and has title and description for root schema as well. That's why it shows in expand collapse section. The schema also has some schema errors. I plan to raise a PR to update that schema in Helm catalog. The gif above is using the custom schema that I created and not the schema that is coming from API. That's why you're seeing that difference.

@christianvogt
Copy link
Contributor

Overall it seems to work well.

Very annoying that the form collapses when switching from yaml to form; discarding the previous expanded states. I think if this merges as is we should raise an issue to capture this request and discuss with UX.

cc @openshift/team-devconsole-ux

@serenamarie125
Copy link
Contributor

This is looking great @rohitkrai03 ! Agree with the comment made by @christianvogt around remember expansion. Is this being handled differently for the operator backed services/operator OLM based forms?

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2020
@openshift-ci-robot openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 26, 2020
@rohitkrai03
Copy link
Contributor Author

This is looking great @rohitkrai03 ! Agree with the comment made by @christianvogt around remember expansion. Is this being handled differently for the operator backed services/operator OLM based forms?

@serenamarie125 Even OLM forms don't have a way to keep the expansion state while switching between form and yaml views. I can create a issue to look into this later.

@rohitkrai03
Copy link
Contributor Author

Comment on lines +3 to +4
import * as Ajv from 'ajv';
import { JSONSchema6 } from 'json-schema';
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have dependencies on these these 2 packages in our package.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

json-schema is being used just for its JSONSchema6 type definition all over the console. Is it necessary to add it to package.json if only type definition is being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

react-jsonschema-form has a dependency on ajv but it's probably better to make it a direct dependency for dev-console package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated package.json with new dependencies.

@christianvogt
Copy link
Contributor

/lgtm
/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, parvathyvr, rohitkrai03

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 29, 2020
@openshift-merge-robot openshift-merge-robot merged commit 92c81e4 into openshift:master Jun 29, 2020
@spadgett spadgett added this to the v4.6 milestone Jul 8, 2020
@rohitkrai03 rohitkrai03 deleted the helm-dynamic-form branch July 24, 2021 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console component/olm Related to OLM component/shared Related to console-shared lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants