Bug 1867824 - The "ordering of fields" on Operand creation form does not behave as intended
Summary: The "ordering of fields" on Operand creation form does not behave as intended
Keywords:
Status: VERIFIED
Alias: None
Product: OpenShift Container Platform
Classification: Red Hat
Component: Management Console
Version: 4.6
Hardware: Unspecified
OS: Unspecified
medium
medium
Target Milestone: ---
: 4.6.0
Assignee: Jon Jackson
QA Contact: Yadan Pei
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2020-08-10 22:16 UTC by tony.wu
Modified: 2020-09-16 01:34 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
Target Upstream Version:


Attachments (Terms of Use)
0_specDescriptors_ordering__portworx_CSV_for_storageCluster@2x.png (286.37 KB, image/png)
2020-08-10 22:16 UTC, tony.wu
no flags Details
1_current__portworx_storageCluster_form@2x.png (390.89 KB, image/png)
2020-08-10 22:17 UTC, tony.wu
no flags Details
2_in_specDescriptors_ordering__portworx_storageCluster_form@2x.png (405.44 KB, image/png)
2020-08-10 22:17 UTC, tony.wu
no flags Details
Storage Cluster Creation Form (226.78 KB, image/png)
2020-08-31 07:24 UTC, Yadan Pei
no flags Details
Storage Cluster Creation Form Correct Order (266.65 KB, image/jpeg)
2020-09-07 02:27 UTC, Yadan Pei
no flags Details
Correct Order in Form when all field group have correct path (210.86 KB, image/png)
2020-09-15 07:13 UTC, Yadan Pei
no flags Details


Links
System ID Priority Status Summary Last Updated
Github openshift console pull 6329 None closed Bug 1867824: Fix dynamic form field ordering logic 2020-09-16 01:31:25 UTC
Github openshift console pull 6600 None closed Bug 1867824: Follow on fix for operand form field sort order. 2020-09-16 01:31:25 UTC

Description tony.wu 2020-08-10 22:16:45 UTC
Created attachment 1711013 [details]
0_specDescriptors_ordering__portworx_CSV_for_storageCluster@2x.png

Description of problem:

Migrated from Jira bug:
https://issues.redhat.com/browse/CONSOLE-1815

When checking the Operator's specDescriptors in the CSV file, it's noticed the form field ordering is not as it intended to be. 

See "Ordering of Form Fields" details in the doc:
https://github.com/openshift/console/tree/master/frontend/packages/operator-lifecycle-manager/src/components/descriptors#ordering-of-form-fields
---
The ordering of form fields depends on whether a field has a specDescriptor, is "required" (a required: property in the schema), "optional", or "advanced" (assigned with an advanced specDecriptor). In general, fields with specDescriptors will be sorted higher than those without and required fields will be sorted higher than optional fields. The full set of sorting rules are as follows:
1. "required" fields with specDescriptors other than advanced
2. "required" fields without specDescriptors
3. "optional" fields with specDescriptors other than advanced
4. "optional" fields without specDescriptors
5. Fields with advanced specDescriptors wrapped in the 'Advanced Configuration' group
---


Version-Release number of selected component (if applicable):
OCP 4.6 and 4.5

How reproducible:
100%

Steps to Reproduce:
1. Install "Portworx Operator" from 'OperatorHub' page (as an example)
2. Go to 'Installed Operators' page and click on '+Create instance' link for "StorageCluster" Operand
3. See the creation form view

Actual results:
See the attached image: 1_current__portworx_storageCluster_form@2x.png


Expected results:
See the attached image: 2_in_specDescriptors_ordering__portworx_storageCluster_form@2x.png

as the specDescriptors in Portworx Operator's CSV is as in:
0_specDescriptors_ordering__portworx_CSV_for_storageCluster@2x.png


Additional info:

Comment 5 Yadan Pei 2020-08-26 08:39:57 UTC
Hi Tony,

Could you please tell me how to get 'Portworx Operator' ? By default the operator is not showing up in OperatorHub in 4.6. I think Portworx Operator maybe a perfect operator to verify this bug.

Comment 6 Yadan Pei 2020-08-31 07:24:08 UTC
Given following specDescriptors for StorageCluster: 
        specDescriptors:
          - description: The docker image name and version of Portworx Enterprise.
            displayName: Image   ==> 1
            path: image
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:text'
          - description: >-
              It is a reference to a secret in the same namespace as the
              StorageCluster. This secret is used to pull images from a private
              registry.
            displayName: Private Registry Image Pull Secret. ==> 2 
            path: imagePullSecret
            x-descriptors:
              - 'urn:alm:descriptor:io.kubernetes:Secret'
              - 'urn:alm:descriptor:com.tectonic.ui:fieldGroup:image'
          - description: >-
              CustomImageRegistry is a custom container registry server (may
              include repository) that will be used instead of index.docker.io
              to download Docker images. (Example: myregistry.net:5443 or
              myregistry.com/myrepository)
            displayName: Custom Image Registry ==> 3
            path: customImageRegistry
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:text'
              - 'urn:alm:descriptor:com.tectonic.ui:fieldGroup:image'
          - description: >-
              Autopilot automatically expands PVCs and Portworx storage based on
              the current usage.
            displayName: Enable Autopilot  ==> grouping in autopilot
            path: autopilot.enable
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:advanced'
              - 'urn:alm:descriptor:com.tectonic.ui:booleanSwitch'
          - description: >-
              The secrets provider which will contain secrets that are needed by
              Portworx for features like volume encryption, cloudsnaps, etc.
            displayName: Secrets Provider 
            path: secretsProvider
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:advanced'
              - 'urn:alm:descriptor:com.tectonic.ui:select:k8s'
              - 'urn:alm:descriptor:com.tectonic.ui:select:vault'
              - 'urn:alm:descriptor:com.tectonic.ui:select:aws-kms'
              - 'urn:alm:descriptor:com.tectonic.ui:select:azure-kv'
              - 'urn:alm:descriptor:com.tectonic.ui:select:ibm-kp'
          - description: This controls the delete strategy of the Portworx cluster.
            displayName: DeleteStrategy  ==> grouping in deletestrategy
            path: deleteStrategy.type
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:advanced'
              - 'urn:alm:descriptor:com.tectonic.ui:select:Uninstall'
              - 'urn:alm:descriptor:com.tectonic.ui:select:UninstallAndWipe'
          - description: >-
              It is the pull policy for the image. Accepts one of Always, Never,
              IfNotPresent. Defaults to Always.
            displayName: Image Pull Policy   ==> 4
            path: imagePullPolicy
          - description: Expose Portworx metrics to Prometheus.
            displayName: Enable Metrics  ==> grouping in Monitoring
            path: monitoring.enableMetrics
          - description: The network interface to be used by Portworx for data traffic.
            displayName: Data Interface  ==> grouping in Network
            path: network.dataInterface
          - description: >-
              The network interface to be used by Portworx for management
              traffic.
            displayName: Management Interface ==> grouping in Network
            path: network.mgmtInterface
          - description: It is the starting port in the range of ports used by Portworx.
            displayName: Start Port  ==> 5
            path: startPort
          - description: Revision history limit is the number of old histories to retain.
            displayName: Revision History Limit ==> 6
            path: revisionHistoryLimit
          - description: >-
              Version is a read-only field. It contains the current version of
              Portworx.
            displayName: Version ==> 7
            path: version


There are three fields with 'urn:alm:descriptor:com.tectonic.ui:advanced' descriptors and the order is:
          - description: >-
              Autopilot automatically expands PVCs and Portworx storage based on
              the current usage.
            displayName: Enable Autopilot  ==> grouping in autopilot
            path: autopilot.enable
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:advanced'
              - 'urn:alm:descriptor:com.tectonic.ui:booleanSwitch'
          - description: >-
              The secrets provider which will contain secrets that are needed by
              Portworx for features like volume encryption, cloudsnaps, etc.
            displayName: Secrets Provider 
            path: secretsProvider
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:advanced'
              - 'urn:alm:descriptor:com.tectonic.ui:select:k8s'
              - 'urn:alm:descriptor:com.tectonic.ui:select:vault'
              - 'urn:alm:descriptor:com.tectonic.ui:select:aws-kms'
              - 'urn:alm:descriptor:com.tectonic.ui:select:azure-kv'
              - 'urn:alm:descriptor:com.tectonic.ui:select:ibm-kp'
          - description: This controls the delete strategy of the Portworx cluster.
            displayName: DeleteStrategy  ==> grouping in deletestrategy
            path: deleteStrategy.type
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:advanced'
              - 'urn:alm:descriptor:com.tectonic.ui:select:Uninstall'
              - 'urn:alm:descriptor:com.tectonic.ui:select:UninstallAndWipe'

in the operand creation form page, the correct order based on my understanding should be : Autopilot -> Delete Strategy -> Secrets Provider comes at the bottom of the form in 'Advanced Options', but the curret order is: Delete Strategy -> Autopilot -> Secrets Provider comes at the bottom of the form in 'Advanced Options'

@Jon, can you help confirm?

Comment 7 Yadan Pei 2020-08-31 07:24:54 UTC
Created attachment 1713117 [details]
Storage Cluster Creation Form

Comment 8 Jon Jackson 2020-09-01 13:15:05 UTC
(In reply to Yadan Pei from comment #6)
> 
> in the operand creation form page, the correct order based on my
> understanding should be : Autopilot -> Delete Strategy -> Secrets Provider
> comes at the bottom of the form in 'Advanced Options', but the curret order
> is: Delete Strategy -> Autopilot -> Secrets Provider comes at the bottom of
> the form in 'Advanced Options'
> 
> @Jon, can you help confirm?

Advanced descriptors are a special case. The normal sorting rules do not apply. The advanced descriptor will cause the target field to be rendered in an expandable "Advanced Configuration" section which is always rendered at the bottom of it's direct parent.

Comment 9 tony.wu 2020-09-01 20:24:10 UTC
Hey Yadan, 

As Jon mentioned, the fields with "Advanced" descriptor will be rendered at the bottom of its direct parent field group.

You can see more details in
https://github.com/openshift/console/tree/master/frontend/packages/operator-lifecycle-manager/src/components/descriptors#ordering-of-form-fields


Hence, in the examples (with advanced) you attached:

1. 'autopilot.enable' with advanced specDescriptor
--> The 'enable' booleanSwitch field should be rendered in an expandable "Advanced Configuration" section within "Autopilot" Accordion. 
--> @Jon, this seems to be a bug here as the "Advanced Configuration" section isn't showing up within "Autopilot" Accordion. 

2. 'secretsProvider' with advanced specDescriptor
--> The 'secretsProvider' dropdown select menu is rendered in an expandable "Advanced Configuration" section at the "top-level" object, which is the Operand instance itself, i.e. essentially the bottom of this creation form.

3. 'deleteStrategy.type' with advanced specDescriptor
--> The 'deleteStrategy.type' (displayName is a bit confusing as the "DeleteStrategy") dropdown select menu is rendered in an expandable "Advanced Configuration" section at the bottom of its direct parent "Delete Strategy" group.

Comment 10 Yadan Pei 2020-09-02 07:35:15 UTC
Thanks very much Jon and Tony.

I now understand `the fields with "Advanced" descriptor will be rendered at the bottom of its direct parent field group.`. 

I have another question that for the parent field group 'Delete Strategy' and 'Authpolit', why are they in a order of Delete Strategy -> Autopilot, not Autopilot -> Delete Strategy? Since in both CRD schema and CSV specDescriptors, Autopilot seems comes first then Delete Strategy.

Comment 11 Jon Jackson 2020-09-03 17:11:18 UTC
Tony and Yadan, I looked into the issues you both pointed out (field ordering incorrect, Autopilot missing 'Advanced Configuration') and it looks like the root cause for both is a bug in the CSV itself. The descriptor intended for the Autopilot -> Enabled field has an incorrect path. The correct schema path to that field is 'autopilot.enabled', but the CSV defines it as 'autopilot.enable'. Updating the path fixes both issues.

Comment 12 tony.wu 2020-09-03 17:47:19 UTC
It seems that the CSV does use 'autopilot.enabled' as the path. Jon can you help clarify this again in terms of the ordering of the fields? Much appreciated!

Comment 13 Yadan Pei 2020-09-07 02:27:04 UTC
Created attachment 1713909 [details]
Storage Cluster Creation Form Correct Order

After updating patch from 'autopilot.enable' to 'autopilot.enabled', the order becomes Autopilot -> Delete Strategy, and Advanced Configuration in Autopilot is rendered.

Comment 14 Yadan Pei 2020-09-08 01:49:16 UTC
It looks like the issue reported has been fixed. moving to VERIFIED

Let me know if it is wrong

Comment 15 tony.wu 2020-09-08 19:23:08 UTC
Hi Yadan, 

I've chatted with Jon on Sep 3rd and clarified the goal for the "ordering of fields" on Operand creation form is "a parent is considered to have a descriptor if any of it’s children do". You can see the "Example case" in the original JIRA bug for reference:
https://issues.redhat.com/browse/CONSOLE-1815 

Hence, the "Create StorageCluster" form should look like as in the attached mocked screenshot: 2_in_specDescriptors_ordering__portworx_storageCluster_form@2x.png

Jon will be working on a PR later for this so he'll need to tag this BZ. Can you help verify that after the new PR? thanks

Comment 16 Yadan Pei 2020-09-09 01:31:31 UTC
Sure,

Let me move it back to ASSIGNED to catch my attention

Comment 17 Jon Jackson 2020-09-11 21:10:43 UTC
PR is in merge queue.

Comment 19 Yadan Pei 2020-09-15 07:11:56 UTC
1. When specDescriptors is in following order: 
        specDescriptors:
          - description: The docker image name and version of Portworx Enterprise.
            displayName: Image
            path: image
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:text'
          - description: >-
              It is a reference to a secret in the same namespace as the
              StorageCluster. This secret is used to pull images from a private
              registry.
            displayName: Private Registry Image Pull Secret
            path: imagePullSecret
            x-descriptors:
              - 'urn:alm:descriptor:io.kubernetes:Secret'
              - 'urn:alm:descriptor:com.tectonic.ui:fieldGroup:image'
          - description: >-
              CustomImageRegistry is a custom container registry server (may
              include repository) that will be used instead of index.docker.io
              to download Docker images. (Example: myregistry.net:5443 or
              myregistry.com/myrepository)
            displayName: Custom Image Registry
            path: customImageRegistry
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:text'
              - 'urn:alm:descriptor:com.tectonic.ui:fieldGroup:image'
          - description: >-      
              Autopilot automatically expands PVCs and Portworx storage based on
              the current usage.
            displayName: Enable Autopilot
            path: autopilot.enable         ====> Autopilot group with incorrect path
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:advanced'
              - 'urn:alm:descriptor:com.tectonic.ui:booleanSwitch'
          - description: >-
              The secrets provider which will contain secrets that are needed by
              Portworx for features like volume encryption, cloudsnaps, etc.
            displayName: Secrets Provider
            path: secretsProvider
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:advanced'
              - 'urn:alm:descriptor:com.tectonic.ui:select:k8s'
              - 'urn:alm:descriptor:com.tectonic.ui:select:vault'
              - 'urn:alm:descriptor:com.tectonic.ui:select:aws-kms'
              - 'urn:alm:descriptor:com.tectonic.ui:select:azure-kv'
              - 'urn:alm:descriptor:com.tectonic.ui:select:ibm-kp'
          - description: This controls the delete strategy of the Portworx cluster.
            displayName: DeleteStrategy  
            path: deleteStrategy.type          ====> DeleteStrategy group with correct path
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:advanced'
              - 'urn:alm:descriptor:com.tectonic.ui:select:Uninstall'
              - 'urn:alm:descriptor:com.tectonic.ui:select:UninstallAndWipe'
          - description: >-
              It is the pull policy for the image. Accepts one of Always, Never,
              IfNotPresent. Defaults to Always.
            displayName: Image Pull Policy
            path: imagePullPolicy
          - description: Expose Portworx metrics to Prometheus.   
            displayName: Enable Metrics
            path: monitoring.enableMetrics         ====> Monitoring group with correct path
          - description: The network interface to be used by Portworx for data traffic.   
            displayName: Data Interface
            path: network.dataInterface            ====> Network group with correct path
          - description: >-
              The network interface to be used by Portworx for management
              traffic.
            displayName: Management Interface
            path: network.mgmtInterface        ====> Network group with correct path
          - description: It is the starting port in the range of ports used by Portworx.
            displayName: Start Port
            path: startPort
          - description: Revision history limit is the number of old histories to retain.
            displayName: Revision History Limit
            path: revisionHistoryLimit
          - description: >-
              Version is a read-only field. It contains the current version of
              Portworx.
            displayName: Version
            path: version

The field group order in form view is: DeleteStrategy -> Monitoring -> Network -> Autopilot

2. When specDescritptors is in following order:
        specDescriptors:
          - description: The docker image name and version of Portworx Enterprise.
            displayName: Image
            path: image
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:text'
          - description: >-
              It is a reference to a secret in the same namespace as the
              StorageCluster. This secret is used to pull images from a private
              registry.
            displayName: Private Registry Image Pull Secret
            path: imagePullSecret
            x-descriptors:
              - 'urn:alm:descriptor:io.kubernetes:Secret'
              - 'urn:alm:descriptor:com.tectonic.ui:fieldGroup:image'
          - description: >-
              CustomImageRegistry is a custom container registry server (may
              include repository) that will be used instead of index.docker.io
              to download Docker images. (Example: myregistry.net:5443 or
              myregistry.com/myrepository)
            displayName: Custom Image Registry
            path: customImageRegistry
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:text'
              - 'urn:alm:descriptor:com.tectonic.ui:fieldGroup:image'
          - description: >-      
              Autopilot automatically expands PVCs and Portworx storage based on
              the current usage.
            displayName: Enable Autopilot
            path: autopilot.enabled         ====> Autopilot group with correct path
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:advanced'
              - 'urn:alm:descriptor:com.tectonic.ui:booleanSwitch'
          - description: >-
              The secrets provider which will contain secrets that are needed by
              Portworx for features like volume encryption, cloudsnaps, etc.
            displayName: Secrets Provider
            path: secretsProvider
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:advanced'
              - 'urn:alm:descriptor:com.tectonic.ui:select:k8s'
              - 'urn:alm:descriptor:com.tectonic.ui:select:vault'
              - 'urn:alm:descriptor:com.tectonic.ui:select:aws-kms'
              - 'urn:alm:descriptor:com.tectonic.ui:select:azure-kv'
              - 'urn:alm:descriptor:com.tectonic.ui:select:ibm-kp'
          - description: This controls the delete strategy of the Portworx cluster.
            displayName: DeleteStrategy  
            path: deleteStrategy.type          ====> DeleteStrategy group with correct path
            x-descriptors:
              - 'urn:alm:descriptor:com.tectonic.ui:advanced'
              - 'urn:alm:descriptor:com.tectonic.ui:select:Uninstall'
              - 'urn:alm:descriptor:com.tectonic.ui:select:UninstallAndWipe'
          - description: >-
              It is the pull policy for the image. Accepts one of Always, Never,
              IfNotPresent. Defaults to Always.
            displayName: Image Pull Policy
            path: imagePullPolicy
          - description: Expose Portworx metrics to Prometheus.   
            displayName: Enable Metrics
            path: monitoring.enableMetrics         ====> Monitoring group with correct path
          - description: The network interface to be used by Portworx for data traffic.   
            displayName: Data Interface
            path: network.dataInterface            ====> Network group with correct path
          - description: >-
              The network interface to be used by Portworx for management
              traffic.
            displayName: Management Interface
            path: network.mgmtInterface        ====> Network group with correct path
          - description: It is the starting port in the range of ports used by Portworx.
            displayName: Start Port
            path: startPort
          - description: Revision history limit is the number of old histories to retain.
            displayName: Revision History Limit
            path: revisionHistoryLimit
          - description: >-
              Version is a read-only field. It contains the current version of
              Portworx.
            displayName: Version
            path: version
The field group order in form view is: Autopilot -> DeleteStrategy -> Monitoring -> Network 

I think this time we have correct order on form view

This is checked against 4.6.0-0.nightly-2020-09-14-181123. 

Moving to VERIFIED

Comment 20 Yadan Pei 2020-09-15 07:13:16 UTC
Created attachment 1714891 [details]
Correct Order in Form when all field group have correct path

Comment 21 Yadan Pei 2020-09-16 01:34:25 UTC
I forgot to mention that the field group are in correct order( Field groups are NOT all sorted after the rest of the descriptor-defined fields),  they are ordered based on child field descriptor order in descriptor array


Note You need to log in before you can comment on or make changes to this bug.