-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
upalatucci
commented
Mar 22, 2024
@upalatucci: This pull request references CNV-39781 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.16.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
6019c20
to
f5fe99a
Compare
f5fe99a
to
e5ccbff
Compare
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.
Looks very good :)
just a small comment
) => { | ||
if (!searchMACAddress) return true; | ||
|
||
return iface?.['mac-address']?.includes(searchMACAddress) || false; |
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.
Please use FILTER_TYPES.MAC_ADDRESS constant
and the || false
can be omitted
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.
i thought here to return false instead of undefined
if there is no mac-address
the FILTER_TYPES.MAC_ADDRESS
does not have anything to do with the mac-address
property. It's the filter id :D
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.
you got the first if, which will return true when searchMACAddress is undefined or empty, btw I would use isEmpty there, since this is covered in the first if, the || false should be redundant, please check
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.
Thanks
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.
the || false is if the interface has no mac address. It's not covered by the first if. It's a different obj.
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.
@avivtur fixed the first if using isEmpty
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hstastna, upalatucci 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 |
/hold |
e5ccbff
to
5af7dab
Compare
/lgtm |
/unhold |