Merge ~jfguedez/juju-lint:bug/1944406 into juju-lint:master

[Jose Guedez]

Add a "suffix" option for config checks

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

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

[James Troup (elmo)]

So here's the problem: we're trying to get rid of nrpe-container/nrpe-{host,physical}. Can I suggest instead a change to the nrpe charm which disables these checks if run in a container? See also:

  https://code.launchpad.net/~vultaire/charm-nrpe/+git/charm-nrpe/+merge/407276

[Jose Guedez (jfguedez)]

Hi James,

I updated the code to be able to handle both situations (to account for a transition period for existing setups). Also added several test cases to cover the success/failure cases for the case with and without suffixes.

Please take another look when you have a chance. Thanks.

Unmerged commits

b8b70ad... by Jose Guedez on 2021-09-21

Add a "suffix" option for config checks

The base charm name is always checked if present.

Closes-Bug: 1944406

diff --git a/contrib/includes/base.yaml b/contrib/includes/base.yaml
index bc39a46..d8063eb 100644
--- a/contrib/includes/base.yaml
+++ b/contrib/includes/base.yaml
@@ -8,8 +8,10 @@ config:
  nrpe:
   lacp_bonds:
    neq: ""
+   suffixes: [host, physical]
   netlinks:
    neq: ""
+   suffixes: [host, physical]
  landscape-client:
   disable-unattended-upgrades:
    eq: true
diff --git a/jujulint/lint.py b/jujulint/lint.py
index 58ce1f1..6c425bd 100755
--- a/jujulint/lint.py
+++ b/jujulint/lint.py
@@ -161,7 +161,9 @@ class Linter:
             for sub in subordinates:
                 if sub in self.model.subs_on_machines[machine]:
                     charm = self.model.app_to_charm[sub]
-                    allow_multiple = self.lint_rules['subordinates'][charm].get("allow-multiple")
+                    allow_multiple = self.lint_rules["subordinates"][charm].get(
+                        "allow-multiple"
+                    )
                     if not allow_multiple:
                         self.model.duelling_subs.setdefault(sub, set())
                         self.model.duelling_subs[sub].add(machine)
@@ -341,20 +343,42 @@ class Linter:
             )
         return False
 
-    def check_config(self, name, config, rules):
+    def check_config(self, app_name, config, rules):
         """Check application against provided rules."""
         rules = dict(rules)
         for rule in rules:
-            self._log_with_header("Checking {} for configuration {}".format(name, rule))
+            self._log_with_header(
+                "Checking {} for configuration {}".format(app_name, rule)
+            )
+
+            # Handle app suffix for config checks. If the suffix is provided
+            # and it does not match, then we skip the check. LP#1944406
+            # The base charm name is always checked if present.
+            suffixes = rules[rule].pop("suffixes", [])
+            if suffixes:
+                charm_name = self.model.app_to_charm[app_name]
+                target_app_names = [
+                    "{}-{}".format(charm_name, suffix) for suffix in suffixes
+                ]
+                target_app_names.append(charm_name)
+
+                if app_name not in target_app_names:
+                    self._log_with_header(
+                        "The app name didn't match any name target for this charm: '{}' (skipping check)".format(
+                            app_name
+                        )
+                    )
+                    continue
+
             for check_op, check_value in rules[rule].items():
                 # check_op should be the operator name, e.g. (eq, neq, gte, isset)
                 if check_op in VALID_CONFIG_CHECKS:
                     check_method = getattr(self, check_op)
-                    check_method(name, check_value, rule, config)
+                    check_method(app_name, check_value, rule, config)
                 else:
                     self._log_with_header(
                         "Application {} has unknown check operation for {}: {}.".format(
-                            name,
+                            app_name,
                             rule,
                             check_op,
                         ),
@@ -429,9 +453,7 @@ class Linter:
                         self._log_with_header("... matched, not wanted on this host")
                         continue
                 elif where == "host only":
-                    self._log_with_header(
-                        "requirement is 'host only' form...."
-                    )
+                    self._log_with_header("requirement is 'host only' form....")
                     if utils.is_container(machine):
                         self._log_with_header("... and we are a container, checking")
                         # XXX check alternate names?
@@ -442,9 +464,7 @@ class Linter:
                         continue
                     self._log_with_header("... and we are a host, will fallthrough")
                 elif where == "metal only":
-                    self._log_with_header(
-                        "requirement is 'metal only' form...."
-                    )
+                    self._log_with_header("requirement is 'metal only' form....")
                     if not utils.is_metal(machine, machines_data.get(machine, {})):
                         self._log_with_header("... and we are not a metal, checking")
                         if required_sub in present_subs:
@@ -483,10 +503,7 @@ class Linter:
                         looking_for = "{}-{}".format(required_sub, suffix)
                         self._log_with_header("-> Looking for {}".format(looking_for))
                         if looking_for in present_subs:
-                            self.logger.debug("-> FOUND!!!")
-                            self.cloud_name,
-                            self.controller_name,
-                            self.model_name,
+                            self._log_with_header("-> FOUND!!!")
                             found = True
                     if not found:
                         for sub in present_subs:
diff --git a/tests/test_jujulint.py b/tests/test_jujulint.py
index c74c550..d865c63 100644
--- a/tests/test_jujulint.py
+++ b/tests/test_jujulint.py
@@ -433,6 +433,74 @@ class TestLinter:
         assert errors[0]["expected_value"] is False
         assert errors[0]["actual_value"] is True
 
+    def test_config_eq_suffix_match(self, linter, juju_status):
+        """Test the config condition 'eq'. when suffix matches."""
+        linter.lint_rules["config"] = {
+            "ubuntu": {"fake-opt": {"eq": False, "suffixes": ["host", "physical"]}}
+        }
+        juju_status["applications"]["ubuntu"]["options"] = {"fake-opt": True}
+        juju_status["applications"]["ubuntu-host"] = juju_status["applications"].pop(
+            "ubuntu"
+        )
+        linter.do_lint(juju_status)
+
+        errors = linter.output_collector["errors"]
+        assert len(errors) == 1
+        assert errors[0]["id"] == "config-eq-check"
+        assert errors[0]["application"] == "ubuntu-host"
+        assert errors[0]["rule"] == "fake-opt"
+        assert errors[0]["expected_value"] is False
+        assert errors[0]["actual_value"] is True
+
+    def test_config_eq_suffix_match_charm_name(self, linter, juju_status):
+        """Test the config condition 'eq'. when suffix and base charm name."""
+        linter.lint_rules["config"] = {
+            "ubuntu": {"fake-opt": {"eq": False, "suffixes": ["host", "physical"]}}
+        }
+        juju_status["applications"]["ubuntu"]["options"] = {"fake-opt": True}
+        linter.do_lint(juju_status)
+
+        errors = linter.output_collector["errors"]
+        assert len(errors) == 1
+        assert errors[0]["id"] == "config-eq-check"
+        assert errors[0]["application"] == "ubuntu"
+        assert errors[0]["rule"] == "fake-opt"
+        assert errors[0]["expected_value"] is False
+        assert errors[0]["actual_value"] is True
+
+    def test_config_eq_suffix_skip(self, linter, juju_status):
+        """Test the config condition 'eq'. when suffix doesn't match."""
+        linter.lint_rules["config"] = {
+            "ubuntu": {"fake-opt": {"eq": False, "suffixes": ["host", "physical"]}}
+        }
+        juju_status["applications"]["ubuntu"]["options"] = {"fake-opt": True}
+        juju_status["applications"]["ubuntu-container"] = juju_status[
+            "applications"
+        ].pop("ubuntu")
+        linter.do_lint(juju_status)
+
+        errors = linter.output_collector["errors"]
+        assert not errors
+
+    def test_config_eq_no_suffix_check_all(self, linter, juju_status):
+        """Test the config condition 'eq'. when no suffix all should be checked."""
+        linter.lint_rules["config"] = {
+            "ubuntu": {"fake-opt": {"eq": False}}
+        }
+        juju_status["applications"]["ubuntu"]["options"] = {"fake-opt": True}
+        juju_status["applications"]["ubuntu-host"] = juju_status["applications"].pop(
+            "ubuntu"
+        )
+        linter.do_lint(juju_status)
+
+        errors = linter.output_collector["errors"]
+        assert len(errors) == 1
+        assert errors[0]["id"] == "config-eq-check"
+        assert errors[0]["application"] == "ubuntu-host"
+        assert errors[0]["rule"] == "fake-opt"
+        assert errors[0]["expected_value"] is False
+        assert errors[0]["actual_value"] is True
+
     def test_config_neq_valid(self, linter, juju_status):
         """Test the config condition 'neq'."""
         linter.lint_rules["config"] = {"ubuntu": {"fake-opt": {"neq": "foo"}}}