Merge ~gabrielcocenza/juju-lint:bug/1939748 into juju-lint:master

[Gabriel Cocenza]

added new config check "search" using regex.

- updated openstack rules for rabbitmq-server to check queue_thresholds.

- added unit tests for "search" config check.

LP #1939748

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

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

[Martin Kalcok (martin-kalcok)]

I'm not sure I'm completely convinced with the `inside` operator. I find the naming itself bit confusing. Current syntax

config_name:
  inside: "some_string"

sounds like the config value should be "substring" of the value that's in the `inside` directive. Alternative name could be "includes" ?

Secondly, If we want to add more flexible option than those that currently exist, maybe we should go for a operator that would match regexp. Example:

config_name:
  match: "regexp"

this would imo add more flexibility than `inside`.

I would also maybe check with the bootstack team about how often they set specific thresholds for individual queues. Because if most of the deployments just setup thresholds with wildcards, we can fix this by simply using.

queue_thresholds:
  eq: "[['\*', '\*', 25000, 27500]]"

That way we could exclude the discussion about the new operator to new ticket/issue.

review: Needs Information

[Gabriel Cocenza (gabrielcocenza)]

Thanks for your review Martin.

I do agree with you that inside wasn't the best naming for that and the regex can be better explored for other use cases that might be necessary to the future.

I just changed the name from "match" as you proposed to "search" because the operator `eq` uses `re.match` in the `helper_operator_eq_check` and can be a little confusing for someone that is looking the code. The new `search` operator uses `re.search` so I think the naming might be better at this situation.

[Paul Goins (vultaire)]

I generally like the change, and it makes sense having a regex match.

My main concern is re: a few of the strings which will be output on error.

I'll give a +1 - one more +1 is still required. However, I left a comment which I hope you'll consider.

review: Approve

[Gabriel Cocenza (gabrielcocenza)]

Thanks for your review Paul. I agree that the messages were awkward when using the `check_config_generic`

[Paul Goins (vultaire)]

I like it. +1; thank you!

review: Approve

[Martin Kalcok (martin-kalcok)]

LGTM as well +1...Thank you Gabriel.

review: Approve

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision ce2c06aec8a1268a9d340c1f05d0a61add1966ff

diff --git a/contrib/includes/openstack.yaml b/contrib/includes/openstack.yaml
index 5235aea..f7b2a85 100644
--- a/contrib/includes/openstack.yaml
+++ b/contrib/includes/openstack.yaml
@@ -21,6 +21,8 @@ openstack config base: &openstack-config-base
  rabbitmq-server:
   cluster-partition-handling:
    eq: "pause_minority"
+  queue_thresholds:
+   search: "\\W\\*, \\W\\*, 25000, 27500"
  keystone:
   token-expiration:
    gte: 86400
diff --git a/jujulint/lint.py b/jujulint/lint.py
index 748fc8a..3b7cfd3 100755
--- a/jujulint/lint.py
+++ b/jujulint/lint.py
@@ -37,7 +37,7 @@ import jujulint.util as utils
 from jujulint.check_spaces import Relation, find_space_mismatches
 from jujulint.logging import Logger
 
-VALID_CONFIG_CHECKS = ("isset", "eq", "neq", "gte")
+VALID_CONFIG_CHECKS = ("isset", "eq", "neq", "gte", "search")
 
 # Generic named tuple to represent the binary config operators (eq,neq,gte)
 ConfigOperator = collections.namedtuple(
@@ -291,6 +291,49 @@ class Linter:
             operator, app_name, check_value, config_key, app_config
         )
 
+    def search(self, app_name, check_value, config_key, app_config):
+        """Scan through the charm config looking for a match using the regex pattern."""
+        actual_value = app_config.get(config_key)
+        if actual_value:
+            if re.search(str(check_value), str(actual_value)):
+                self._log_with_header(
+                    "Application {} has a valid config for '{}': regex {} found at {}".format(
+                        app_name,
+                        config_key,
+                        repr(check_value),
+                        repr(actual_value),
+                    )
+                )
+                return True
+            self.handle_error(
+                {
+                    "id": "config-search-check",
+                    "tags": ["config", "search"],
+                    "description": "Checks for config condition 'search'",
+                    "application": app_name,
+                    "rule": config_key,
+                    "expected_value": check_value,
+                    "actual_value": actual_value,
+                    "message": "Application {} has an invalid config for '{}': regex {} not found at {}".format(
+                        app_name,
+                        config_key,
+                        repr(check_value),
+                        repr(actual_value),
+                    ),
+                }
+            )
+            return False
+
+        self._log_with_header(
+            "Application {} has no config for '{}', can't search the regex pattern {}.".format(
+                app_name,
+                config_key,
+                repr(check_value),
+            ),
+            level=logging.WARN,
+        )
+        return False
+
     def check_config_generic(
         self, operator, app_name, check_value, config_key, app_config
     ):
diff --git a/tests/test_jujulint.py b/tests/test_jujulint.py
index 11cc514..3603146 100644
--- a/tests/test_jujulint.py
+++ b/tests/test_jujulint.py
@@ -603,6 +603,37 @@ applications:
         assert errors[0]["application"] == "ubuntu"
         assert errors[0]["rule"] == "fake-opt"
 
+    def test_config_search_valid(self, linter, juju_status):
+        """Test the config condition 'search' when valid."""
+        linter.lint_rules["config"] = {
+            "ubuntu": {"fake_opt": {"search": "\\W\\*, \\W\\*, 25000, 27500"}}
+        }
+        juju_status["applications"]["ubuntu"]["options"] = {
+            "fake_opt": "[[/, queue1, 10, 20], [\\*, \\*, 25000, 27500]]"
+        }
+        linter.do_lint(juju_status)
+
+        errors = linter.output_collector["errors"]
+        assert len(errors) == 0
+
+    def test_config_search_invalid(self, linter, juju_status):
+        """Test the config condition 'search' when invalid."""
+        linter.lint_rules["config"] = {
+            "ubuntu": {"fake_opt": {"search": "\\W\\*, \\W\\*, 25000, 27500"}}
+        }
+        juju_status["applications"]["ubuntu"]["options"] = {
+            "fake_opt": "[[/, queue1, 10, 20], [\\*, \\*, 10, 20]]"
+        }
+        linter.do_lint(juju_status)
+
+        errors = linter.output_collector["errors"]
+        assert len(errors) == 1
+        assert errors[0]["id"] == "config-search-check"
+        assert errors[0]["application"] == "ubuntu"
+        assert errors[0]["rule"] == "fake_opt"
+        assert errors[0]["expected_value"] == "\\W\\*, \\W\\*, 25000, 27500"
+        assert errors[0]["actual_value"] == "[[/, queue1, 10, 20], [\\*, \\*, 10, 20]]"
+
     def test_parse_cmr_apps_export_bundle(self, linter):
         """Test the charm CMR parsing for bundles."""
         parsed_yaml = {