LOG-4810: Do not change operator if selector value is regex #156
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jgbernalp: This pull request references LOG-4810 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 bug to target the "4.15.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 kubernetes/test-infra repository. |
|
/cherry-pick release-5.8 |
|
@jgbernalp: once the present PR merges, I will cherry-pick it on top of release-5.8 in a new PR and assign it to you. 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. |
|
/cherry-pick release-5.7 |
|
@jgbernalp: once the present PR merges, I will cherry-pick it on top of release-5.7 in a new PR and assign it to you. 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. |
openshift-ci
bot
added
the
approved
web/src/attribute-filters.tsx
Outdated
| const testRegex = /\\|\[|\{|\(|\)|\^|\$|\.|\*|\?|\||\+|\/|\]/; | ||
| return testRegex.test(value); |
There was a problem hiding this comment.
This looks like it will return true for a string that contains any one of the special characters (\, [, {, (, ), ^, $, ., *, ?, |, +, /, ]), but don't we need to support exact matching against string values that contain these characters?
There was a problem hiding this comment.
In this case we are only matching k8s names which exclude these characters as they are DNS names. The only exception is dot '.'. I adjusted the code to consider this case and added some tests and comments for clarity.
There was a problem hiding this comment.
Thanks for the explanation!
If we use a character class (square brackets), I think only the \ and ] need to be escaped. So like [{}()[\]^$*+?|/\\]|\.[*+?], which I think is at least a bit easier to read.
Note that I also added the } character in that regex, which I guess should be included for completeness.
There was a problem hiding this comment.
Thnx I changed the regex with your suggestion and is more readable now
web/src/attribute-filters.tsx
Outdated
| if (isValueARegex(value)) { | ||
| return { | ||
| label, | ||
| operator: '=~', |
There was a problem hiding this comment.
Nit: How about operator: isValueARegex(value) ? '=~' : '=', to reduce noise and make it clearer that only the operator depends on whether the value is a regex.
jgbernalp
force-pushed
the
fix-regex-operator
branch
from
fdea0cf to
b0f2ed4
Compare
jgbernalp
force-pushed
the
fix-regex-operator
branch
from
b0f2ed4 to
2ea46bd
Compare
|
@jgbernalp: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgbernalp, kyoto 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 |
|
@jgbernalp: new pull request created: #159 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. |
|
@jgbernalp: new pull request created: #160 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. |
If a matcher value is changed manually into a regex, do not alter the operator so the query results filter as expected by the user