Modified array field group descriptor to allow add or remove fields f… #4047
Conversation
|
Hi @yzhao583. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
openshift-ci-robot
added
needs-ok-to-test
|
/ok-to-test |
openshift-ci-robot
added
ok-to-test
|
/assign @TheRealJon |
|
Hey @yzhao583 , it's noticed the "add/remove" behavior in this PR is a bit counterintuitive.. The "add/remove" behavior we were thinking is: Can you help check this out? Thanks! |
|
For any others reviewing, this is the current functionality: And this is more along the lines of what Tony and I discussed: |
yzhao583
force-pushed
the
modify-arrayFieldGroup-descriptor-to-allow-add-or-remove-fields-from-the-group
branch
from
b4b0bd2 to
9bf84e9
Compare
|
@tlwu2013 @TheRealJon @spadgett Thanks for the clarification. I really appreciate that. However, considering the time line, can we merge this PR as the initial implementation of this feature? I will submit another PR to fix the bug ASAP. Thanks. |
TheRealJon
left a comment
TheRealJon
left a comment
There was a problem hiding this comment.
/approve
/lgtm
I think this is ok to merge.
As follow on, I think we could improve readability by breaking out a few more helper functions, adding some comments, using string interpolation instead of concat where appropriate, and incorporating more ES6 syntax for manipulating and referencing collections.
|
|
||
| const getArrayFieldGroups = () => { | ||
| const arrayFieldGroupList = fields.reduce( | ||
| (groups, field) => |
There was a problem hiding this comment.
Should probably just use a const to hold the arrayFieldGroup value for the current field. It will make this easier to read an only iterate over the capabilities array once:
(groups, field) => {
const arrayFieldGroup = field.capabilities.find((c) => c.startsWith(SpecCapability.arrayFieldGroup);
return arrayFiledGroup ? groups.add(arrayFieldGroup) : groups;
}| const getArrayFieldGroups = () => { | ||
| const arrayFieldGroupList = fields.reduce( | ||
| (groups, field) => | ||
| field.capabilities.find((c) => c.startsWith(SpecCapability.arrayFieldGroup)) |
There was a problem hiding this comment.
No change needed now, but we should create a helper function to retrieve the arrayFieldGroup descriptor from a field.
| .filter((f) => !_.isNil(inputFor(f))); | ||
|
|
||
| return ( | ||
| !_.isEmpty(fieldList) && ( |
There was a problem hiding this comment.
Not new, but just noticing that this could throw a runtime error if fieldList is empty. Need to explicitly return null if it is.
| return ( | ||
| !_.isEmpty(fieldList) && ( | ||
| <div id={group} key={group}> | ||
| {[...arrayFieldGroups].filter((fieldGroup) => |
There was a problem hiding this comment.
No need for spread operator here.
| const groupNumIndex = pathInfoList.indexOf(groupInfo); | ||
| const groupIndex = group.split(groupName.concat(':'))[1]; | ||
| const newGroupName = groupName | ||
| .concat('[') |
There was a problem hiding this comment.
Don't need to change now, but use string interpolation instead of chained concat.
|
/approve |
spadgett
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: spadgett, TheRealJon, yzhao583 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
6 participants
This PR contains the change for the new "arrayFieldGroup" Spec Descriptor for the OLM Create Operand Form. With this descriptor, the Field Group can be added to the Field Group Array or removed from the Field Group Array.
Concepts
ArrayFieldGroup: ArrayFieldGroup is a Spec Descriptor which allows you to specify a set of fields together as an array item. Nested fields will automatically be grouped using the CRD’s OpenAPI validation.
Field Group: Field Group is a set of fields.
Field Group Array: Field Group Array is an array containing many Field Group.
X-descriptor Structure
The “arrayFieldGroup” x-descriptor allows users to specify a Field Group Array. In the array, each element is a Field Group, users can add/remove Field Group. The following image demonstrates the structure of the Descriptor.
Usage
Add the “arrayFieldGroup” x-descriptor into the “x-descriptors” array of the field object.
Replace the “FIELD_GROUP_ARRAY_NAME” with the real name.