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
Bug 1890180: Bug 1913969: Fix edge case exception for fieldDependency spec descriptor and add support for non-sibling control fields. #7957
Conversation
The form field sort ordering logic was causing a stack overflow when the control field an dependent field had the same name. Resolved by keeping track of the current path and control field path to differentiate between the two. This also fixes https://bugzilla.redhat.com/show_bug.cgi?id=1890180
@TheRealJon: This pull request references Bugzilla bug 1913969, which is invalid:
Comment In response to this:
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. |
/bugzilla refresh |
@TheRealJon: This pull request references Bugzilla bug 1913969, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
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. |
/override ci/prow/ceph-storage-plugin The ceph-storage-plugin job should only have run on PRs with changes to |
@spadgett: Overrode contexts on behalf of spadgett: ci/prow/ceph-storage-plugin In response to this:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor nits, but I wouldn't hold the PR on either comment.
/lgtm
// If this property is a dependent, it's weight is based on it's control field | ||
if (controlFieldName) { | ||
return getJSONSchemaPropertySortWeight(controlFieldName, jsonSchema, uiSchema) + offset; | ||
// If this property or it's children have a control field at the current path, it's weight is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// If this property or it's children have a control field at the current path, it's weight is | |
// If this property or its children have a control field at the current path, it's weight is |
.toJS(); | ||
|
||
return { | ||
...(uiOrder.length > 1 && { 'ui:order': uiOrder }), | ||
..._.reduce( | ||
jsonSchema?.properties ?? {}, | ||
(orderAccumulator, propertySchema, propertyName) => { | ||
const descendantOrder = getJSONSchemaOrder(propertySchema, uiSchema?.[propertyName]); | ||
const descendantOrder = getJSONSchemaOrder( | ||
propertySchema as JSONSchema6, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you simply declare the type above in the function definition like
(orderAccumulator, propertySchema: JSONSchema6, propertyName) => {
This is more type safe than using as
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: spadgett, TheRealJon 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 |
@TheRealJon: All pull requests linked via external trackers have merged: Bugzilla bug 1913969 has been moved to the MODIFIED state. In response to this:
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. |
@TheRealJon: All pull requests linked via external trackers have merged: Bugzilla bug 1890180 has been moved to the MODIFIED state. In response to this:
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. |
/bugzilla refresh |
@TheRealJon: Bugzilla bug 1890180 is in an unrecognized state (MODIFIED) and will not be moved to the MODIFIED state. In response to this:
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. |
Fixes #8120 |
The form field sort ordering logic was causing a stack overflow when the control field an dependent field had the same name. Resolved by keeping track of the current path and control field path to differentiate between the two. This also fixes https://bugzilla.redhat.com/show_bug.cgi?id=1890180