Merge ~mertkirpici/juju-lint:lp/1993735 into juju-lint:master

Proposed by Mert Kirpici
Status: Merged
Approved by: Robert Gildein
Approved revision: ff6c048579ecf261cc2cdb6b3bafecf644af4b71
Merged at revision: 1108dacb9fe66ef726d4764bcd2a3703ef6bfa14
Proposed branch: ~mertkirpici/juju-lint:lp/1993735
Merge into: juju-lint:master
Diff against target: 70 lines (+22/-10)
2 files modified
jujulint/lint.py (+15/-0)
tests/unit/test_jujulint.py (+7/-10)
Reviewer Review Type Date Requested Status
🤖 prod-jenkaas-bootstack (community) continuous-integration Approve
Robert Gildein Approve
Gabriel Cocenza Approve
BootStack Reviewers Pending
Review via email: mp+432462@code.launchpad.net

Commit message

Close LP #1993735

Description of the change

The callback function `helper_operator_eq_check` for the eq operator, has an overloaded behavior. It first tries regex matching and if that is not successful, it tests for regular string equality checking.

This caused some issues when testing against an empty string. A check value of empty string used to match any kind of input, which is quite counter-intuitive.

With this change we are adding an exception for the empty string case to force string equality check in that case. Also adding a deprecation notice for the overloaded regex checking behavior to inform the users that it will be removed in the future.

To post a comment you must log in.
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

Revision history for this message
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Robert Gildein (rgildein) wrote :

One suggestion and I think that we need to rewrite
`test_helper_operator_check` test function.

review: Needs Fixing
Revision history for this message
Mert Kirpici (mertkirpici) wrote :

Hi Robert thanks for the review and suggestions. I applied both of them in the update.

Revision history for this message
Gabriel Cocenza (gabrielcocenza) wrote :

LGTM

review: Approve
Revision history for this message
Robert Gildein (rgildein) wrote :

LGTM

review: Approve
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote :

Change successfully merged at revision 1108dacb9fe66ef726d4764bcd2a3703ef6bfa14

Revision history for this message
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/jujulint/lint.py b/jujulint/lint.py
index f73b7ea..4219b2f 100755
--- a/jujulint/lint.py
+++ b/jujulint/lint.py
@@ -60,6 +60,16 @@ ConfigOperator = collections.namedtuple(
6060
61def helper_operator_eq_check(check_value, actual_value):61def helper_operator_eq_check(check_value, actual_value):
62 """Perform the actual equality check for the eq/neq rules."""62 """Perform the actual equality check for the eq/neq rules."""
63 # An empty string regex matches any string. Therefore
64 # we are handling the empty string as a special case for now
65 # as a workaround.
66 # The overloaded regex matching behavior will be removed in a
67 # future version.
68 #
69 # See LP #1993735
70 if check_value == "":
71 return check_value == actual_value
72
63 match = False73 match = False
64 try:74 try:
65 match = re.match(re.compile(str(check_value)), str(actual_value))75 match = re.match(re.compile(str(check_value)), str(actual_value))
@@ -126,6 +136,11 @@ class Linter:
126136
127 # collect errors only for non-text output (e.g. json)137 # collect errors only for non-text output (e.g. json)
128 self.collect_errors = True if self.output_format != "text" else False138 self.collect_errors = True if self.output_format != "text" else False
139 self.logger.warn(
140 "Regex autodetection feature of the eq operator is deprecated. "
141 "It will only check for equality in the future. "
142 "Please use the search operator for regex checks."
143 )
129144
130 def read_rules(self):145 def read_rules(self):
131 """Read and parse rules from YAML, optionally processing provided overrides."""146 """Read and parse rules from YAML, optionally processing provided overrides."""
diff --git a/tests/unit/test_jujulint.py b/tests/unit/test_jujulint.py
index c87722d..9f95f43 100644
--- a/tests/unit/test_jujulint.py
+++ b/tests/unit/test_jujulint.py
@@ -1540,25 +1540,22 @@ applications:
1540 logger_mock.warn.assert_called_once_with(expected_msg)1540 logger_mock.warn.assert_called_once_with(expected_msg)
15411541
1542 @pytest.mark.parametrize(1542 @pytest.mark.parametrize(
1543 "regex_error, check_value, actual_value",1543 "expected_result, check_value, actual_value",
1544 [1544 [
1545 (True, "same", "same"),1545 (True, "same", "same"),
1546 (True, "same", "different"),
1547 (False, "same", "same"),
1548 (False, "same", "different"),1546 (False, "same", "different"),
1547 (False, "same", "Same"),
1548 (True, "[sS]ame", "Same"),
1549 (True, "[same", "[same"),
1550 (False, "", "foo"),
1551 (True, "", ""),
1549 ],1552 ],
1550 )1553 )
1551 def test_helper_operator_check(1554 def test_helper_operator_check(
1552 self, regex_error, check_value, actual_value, mocker1555 self, expected_result, check_value, actual_value, mocker
1553 ):1556 ):
1554 """Test comparing values using "helper_operator_check()" function."""1557 """Test comparing values using "helper_operator_check()" function."""
1555 if regex_error:
1556 mocker.patch.object(lint.re, "match", side_effect=lint.re.error(""))
1557
1558 expected_result = check_value == actual_value
1559
1560 result = lint.helper_operator_eq_check(check_value, actual_value)1558 result = lint.helper_operator_eq_check(check_value, actual_value)
1561
1562 assert bool(result) == expected_result1559 assert bool(result) == expected_result
15631560
1564 @pytest.mark.parametrize(1561 @pytest.mark.parametrize(

Subscribers

People subscribed via source and target branches