Bug 1846863: Fix runtime error on legacy operand form #5737
Conversation
openshift-ci-robot
added
bugzilla/severity-high
|
@TheRealJon: This pull request references Bugzilla bug 1846863, 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. |
|
/hold Was unable to reproduce the original bug, but this is my best guess for the cause since it is the only place in the olm package where we were trying to access a |
openshift-ci-robot
added
the
do-not-merge/hold
|
@TheRealJon Can you force an error by injecting some invalid data before create? Then you might be able to reproduce. |
|
Even just k8sCreate(model, {})might be enough to see if it blows up on the error handling. |
|
@spadgett Yeah, I'm sure I could make it blow up manually. I just want to verify that this PR actually fixes the linked bug before specifically merging it as a fix. |
|
@TheRealJon: This pull request references Bugzilla bug 1846863, which is valid. 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. |
|
/hold cancel Was able to duplicate and confirm a fix for the linked bug. Updated the description to reflect the other changes. |
openshift-ci-robot
removed
the
do-not-merge/hold
|
cc @spadgett |
| @@ -520,7 +519,7 @@ export const DEPRECATED_CreateOperandForm: React.FC<OperandFormProps> = ({ | |||
| const existing = immutableFormData.getIn([...pathToArray(pathBeforeIndex), 0]); | |||
| const item = Immutable.Map(existing || {}).setIn(pathToArray(pathAfterIndex), value); | |||
| const list = Immutable.List([item]); | |||
| onChange(immutableFormData.setIn(pathToArray(pathBeforeIndex), list)); | |||
| onChange(immutableFormData.setIn(pathToArray(pathBeforeIndex), list).toJS()); | |||
There was a problem hiding this comment.
This is not technically part of the bug fix, but was a problem that I noticed while scanning the code. onChange shouldn't return an Immutable.Map instance.
|
/assign @spadgett |
|
[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 |
openshift-ci-robot
added
the
approved
|
@TheRealJon: All pull requests linked via external trackers have merged: openshift/console#5737. Bugzilla bug 1846863 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. |
|
/cherrypick release-4.5 |
|
@jhadvig: new pull request created: #5878 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. |
Legacy operand form k8sCreate catch callback was trying to use
err.json.messagewhich doesn't always exist. Update to useerr.messagewhich should always be defined since it is initialized by the Error constructor. For safe measure, added a fallback value in caseerr.messageis falsy.The legacy operand form was also adding a namespace property to every resources before submitting, which was incorrect. Update to only add namespace if model is namespaced.