[WIP] Cleanup PipelineBuilder validation code

[andrewballantyne]

Fixes:
https://issues.redhat.com/browse/ODC-3165

Analysis / Root cause:
When the Pipeline Builder was put into 4.4 as a feature the week before Feature Freeze, corners were cut to get around some yup & Formik issues. This has caused a very brittle validation schema and served as non-stop issues.

Solution Description:
Rewrite the validation schema to validate the entirety of the Pipeline Builder and remove all custom error code / handling.

Screen shots / Gifs for design review:

@openshift/team-devconsole-ux

Empty Name:

Invalid Name:

Array Param with error & description (breaks from the norm?):

Counter example of first field error:

Successful Resource connections:

Rename a resource; breaks connection:

Update to correct:

Delete resource (breaks connection but no fix):

Unit test coverage report:

• Tests coming

Test setup:

• Need Pipeline Operator
• Need to be on the Pipeline Builder (create Pipeline button on the Pipelines page)

This is my validation path, these should all pass (with the exception of one -- noted below).

1. Name
   1. Empty (error)
   2. Invalid characters (error)
2. Tasks (prompts a silent error for needing at least one)
   1. Visualization
      1. No initial errors; openshift-client (no error)
      2. Initial Resource errors; s2i-nodejs (error)
      3. Initial Param & Resource errors; s2i-java-11 (error, resources over params)
   2. Details
      1. Array Param; ie openshift-client
         1. Add a new row (error); removing empty should clear the error
         2. Emptying the only field there (error)
      2. Param & Resource errors; s2i-java-11
         1. Initial (error, resources)
         2. Clear the Resource error by creating resources (error, params)
         3. Clear the param error by filling in the required fields (no error)
      3. Name
         1. Emptying (error)
         2. Invalid (error)
         3. Doesn’t update unless valid and on blur (to be fixed later)
      4. Resources connections
         1. Add s2i-nodejs (error)
         2. Create two resources; git & image (still in error)
         3. Update both in the details panel (no error)
         4. Change the name of one of the resources in the main form (error)
         5. Update in Details (no error)
         6. Delete resource from main form (error)
         7. No change possible (still in error)
         8. Add openshift-client task (for a positive passing task)
         9. Remove s2i-nodejs (error? — Formik has issues, still looking into it, any further update clears the error)
3. Parameters & Resources should error on blur of any required field (name for both & type for resources)

Browser conformance:

• Chrome
• Firefox
• Safari
• Edge

/kind cleanup
/cc @rohitkrai03

[andrewballantyne]

Some initial comments to help with reviews.

[andrewballantyne]

Odd that this does not fix my update error... Will need to investigate why. cc @rohitkrai03

[rohitkrai03]

You might try moving this to the specific part of the form where validation issue is happening.

[andrewballantyne]

Interesting... figured this was a deep compare... but will look into it. Perhaps there is some React non-triggering happening that could be triggering the issue. I'll investigate, thanks Rohit.

[andrewballantyne]

I'm not happy with this -- open to ideas on how I can keep the width of the inputs the same throughout the sidebar (the string[] guy comes with a "-" sign on the right side and that's 28px wide).

[andrewballantyne]

/cc @abhinandan13jan

Adding Abhi as the second reviewer as he has taken an interest in Pipelines in the past.

[serenamarie125]

@andrewballantyne i'll look through a bit more carefully, but I notice that you use "please" in the messages. Our convention is to omit "please" from the UI, so if you can pull that word out, that would be great!

[andrewballantyne]

@andrewballantyne i'll look through a bit more carefully, but I notice that you use "please" in the messages. Our convention is to omit "please" from the UI, so if you can pull that word out, that would be great!

Fixed that, thanks for the reminder 😄 -- I've updated the description.

@serenamarie125 While I was there looking at the code, I noticed this scenario got missed in refactor and the solution could not be the same... let me explain.

This is what we have today as the initial resting state (immediately after adding the task and opening the sidebar):

However, this is functionality that worked because the field was custom before this work... now we are using the standard form field and it's actually not possible to be in an error state, disabled, and where the user has never touched the field. This seems pretty consistent throughout our add forms as well.

Solution... make it just help text (probably not ideal?):

When you have a field and when you select it... I modified the tooltip to be similar to what I had but slightly different (see description screenshots for the previous look):

[rohitkrai03]

You might try moving this to the specific part of the form where validation issue is happening.

[rohitkrai03]

/assign

[rohitkrai03]

Okay, just went through the PR again and ran it locally. Found a few issues -

Not sure why validation says required for this field.

And i couldn't create any pipeline for some reason. Or even edit already created one. I created a pipeline from git import. Went to edit it and it started showing validation error. Fixed everything up but still got this error -

[rohitkrai03]

If the pipeline is already create with resources and parameters defined in it. Why am I seeing this error when trying to edit it? Should the task be already connected with the resources and parameters?

[andrewballantyne]

I somehow broke logic and didn't test submission after that 🤦‍♂ Ah well, good catch @rohitkrai03 Thanks! I'll fix it. Got name and resource mixed up and it works fine building, but won't pass validation if I submit with inverse of what is needed.

[andrewballantyne]

Not sure why validation says required for this field.

Can't reproduce this

Nvm, I can now... doh on my problem though heh... should be an easy fix.

[andrewballantyne]

Adding a WIP label to hide it from filters for Feature Freeze. Will go in next week.

[andrewballantyne]

Needs to include logic from #5235 when reworked for 4.6

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: andrewballantyne
To complete the pull request process, please assign jhadvig
You can assign the PR to them by writing /assign @jhadvig in a comment when ready.

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

Needs approval from an approver in each of these files:

• frontend/OWNERS

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

[andrewballantyne]

Another PR to merge in #6228

[andrewballantyne]

/hold

[openshift-ci-robot]

@andrewballantyne: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/frontend	7a20104	link	/test frontend
ci/prow/kubevirt-plugin	7a20104	link	/test kubevirt-plugin

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[openshift-ci]

@andrewballantyne: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/kubevirt-plugin	7a20104	link	/test kubevirt-plugin
ci/prow/ceph-storage-plugin	7a20104	link	/test ceph-storage-plugin

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.