Merge ~peppepetra/juju-lint:lp2043115 into juju-lint:master

[Giuseppe Petralia]

Check relations by charm name and not application

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

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

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:cce09acd4c4317fb2c4fd5971d190af99976fc31
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/19/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest31/524/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/761/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/19//rebuild

review: Needs Fixing (continuous-integration)

487201a... by Giuseppe Petralia on 2023-11-10

Add comments

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:487201a4ae87ff46e0afbdfe59666a0641a29c9e
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/22/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest31/527/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/764/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/22//rebuild

review: Needs Fixing (continuous-integration)

47ee7fb... by Giuseppe Petralia on 2023-11-10

Fix unit tests

4096722... by Giuseppe Petralia on 2023-11-11

Update unittests

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:4096722860d84b917113f264593bc916b8eef117
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/27/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest31/573/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/769/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/27//rebuild

review: Needs Fixing (continuous-integration)

66e750a... by Giuseppe Petralia on 2023-11-13

Fix linting

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:66e750a8755c6cd2f8472b0fb276c39c2a6b1b13
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/28/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest31/574/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/770/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-juju-lint-ci/28//rebuild

review: Needs Fixing (continuous-integration)

[Gabriel Cocenza (gabrielcocenza)]

Thanks for this patch.

Please, see my comments to some enhancements.

Thanks

review: Needs Fixing

Unmerged commits

66e750a... by Giuseppe Petralia on 2023-11-13

Fix linting

4096722... by Giuseppe Petralia on 2023-11-11

Update unittests

47ee7fb... by Giuseppe Petralia on 2023-11-10

Fix unit tests

487201a... by Giuseppe Petralia on 2023-11-10

Add comments

cce09ac... by Giuseppe Petralia on 2023-11-09

Check relations by charm name and not application

diff --git a/jujulint/checks/relations.py b/jujulint/checks/relations.py
index 6b85a6e..6ee514b 100644
--- a/jujulint/checks/relations.py
+++ b/jujulint/checks/relations.py
@@ -193,6 +193,7 @@ class RelationRule:
                     relations_app_endpoint_0 = self.input_file.filter_by_relation(
                         {app_to_check_0}, endpoint_to_check_0
                     )
+
                     if app_to_check_1 in relations_app_endpoint_0:
                         self.not_exist_error.append(relation)
 
diff --git a/jujulint/model_input.py b/jujulint/model_input.py
index 8233ebb..51e1cb0 100644
--- a/jujulint/model_input.py
+++ b/jujulint/model_input.py
@@ -36,6 +36,13 @@ class BaseFile:
         """Applications present in the file."""
         return set(self.applications_data.keys())
 
+    @property
+    def charm_names(self) -> Set:
+        """Charms present in the file."""
+        return set(
+            self.applications_data[app]["charm"] for app in self.applications_data
+        )
+
     @staticmethod
     def split_relation(relation: List[List[str]]) -> Tuple[str, str]:
         """Split relations into apps and endpoints.
@@ -78,16 +85,25 @@ class BaseFile:
         # app == "*" means all apps
         # a charm from relation rule can have different app names.
         if app != "*" and app != charm:
-            if app not in self.applications:
+            # Charm name may contain ch: or cs: at the beginning
+            # or -<rev> at the end. Then we check that app name is contained
+            # in charm_name
+            found = False
+            for name in self.charm_names:
+                if app in name:
+                    found = True
+            if not found:
                 LOGGER.warning(f"{app} not found on applications.")
                 return "", ""
 
             # NOTE(gabrielcocenza) it's not always that a bundle will contain all endpoints under "bindings".
             # See LP#1949883 and LP#1990017
             # juju-info is represented by "" on endpoint-bindings
-            if endpoint != "juju-info" and endpoint not in self.applications_data[
-                app
-            ].get(endpoints_key, {}):
+            if (
+                endpoint != "juju-info"
+                and app in self.applications_data
+                and endpoint not in self.applications_data[app].get(endpoints_key, {})
+            ):
                 LOGGER.warning(f"endpoint: {endpoint} not found on {app}")
                 return "", ""
         return app, endpoint
@@ -119,11 +135,25 @@ class BaseFile:
                 if endpoint
                 in self.applications_data.get(app, {}).get(endpoints_key, {})
             }
-        return (
-            set([app])
-            if endpoint in self.applications_data.get(app, {}).get(endpoints_key, {})
-            else set()
-        )
+
+        # Create a dictionary where keys are applications names using charm = app
+        # and values are list of endpoints connected
+        endpoints = {}
+        for application in self.applications_data:
+            if app in self.applications_data[application].get("charm"):
+                endpoints[application] = list(
+                    self.applications_data[application].get(endpoints_key, {}).keys()
+                )
+
+        result = set()
+
+        # return the list of applications which have the relation in place
+        # with the endpoint it is checking.
+        for application in endpoints:
+            if endpoint in endpoints[application]:
+                result.add(application)
+
+        return result
 
     def filter_machines_by_charm(self, charm: str) -> Set:
         """Filter machines that has a specific charm.
@@ -231,10 +261,10 @@ class JujuStatusFile(BaseFile):
         BaseFile.check_app_endpoint_existence, endpoints_key="endpoint-bindings"
     )
 
-    def filter_by_relation(self, apps: Set, endpoint: str) -> Set:
+    def filter_by_relation(self, charms: Set, endpoint: str) -> Set:
         """Filter applications that relate with an endpoint.
 
-        :param apps: Applications to filter relations using the endpoint.
+        :param charms: Charms to filter relations using the endpoint.
         :type apps: Set
         :param endpoint: endpoint of the applications.
         :type endpoint: str
@@ -242,6 +272,12 @@ class JujuStatusFile(BaseFile):
         :rtype: Set
         """
         apps_related = set()
+        apps = set()
+        for charm in charms:
+            for app in self.applications_data:
+                if charm in self.applications_data[app].get("charm", ""):
+                    apps.add(app)
+
         for app in apps:
             relations = self.applications_data.get(app, {}).get("relations", {})
             apps_related.update(relations.get(endpoint, []))
@@ -333,10 +369,10 @@ class JujuBundleFile(BaseFile):
         BaseFile.check_app_endpoint_existence, endpoints_key="bindings"
     )
 
-    def filter_by_relation(self, apps: Set, endpoint: str) -> Set:
+    def filter_by_relation(self, charms: Set, endpoint: str) -> Set:
         """Filter applications that relate with an endpoint.
 
-        :param apps: Applications to filter relations using the endpoint.
+        :param charms: Charms to filter relations using the endpoint.
         :type apps: Set
         :param endpoint: endpoint of the applications.
         :type endpoint: str
@@ -344,6 +380,13 @@ class JujuBundleFile(BaseFile):
         :rtype: Set
         """
         apps_related = set()
+
+        apps = set()
+        for charm in charms:
+            for app in self.applications_data:
+                if charm in self.applications_data[app].get("charm", ""):
+                    apps.add(app)
+
         for relation in self.relations_data:
             for app in apps:
                 app_ep = f"{app}:{endpoint}"
diff --git a/tests/unit/test_input.py b/tests/unit/test_input.py
index f06ff4b..174f1a7 100644
--- a/tests/unit/test_input.py
+++ b/tests/unit/test_input.py
@@ -202,10 +202,7 @@ def test_filter_by_app_and_endpoint(
 def test_filter_by_relation(input_file_type, endpoint, expected_output, input_files):
     """Test filter_by_relation method behave as expected."""
     input_file = input_files[input_file_type]
-    assert (
-        input_file.filter_by_relation(input_file.charm_to_app["nrpe"], endpoint)
-        == expected_output
-    )
+    assert input_file.filter_by_relation(["nrpe"], endpoint) == expected_output
 
 
 @pytest.mark.parametrize(
diff --git a/tests/unit/test_jujulint.py b/tests/unit/test_jujulint.py
index b98474b..3afb836 100644
--- a/tests/unit/test_jujulint.py
+++ b/tests/unit/test_jujulint.py
@@ -1786,8 +1786,8 @@ applications:
         # add a relation rule that doesn't happen in the model
         linter.lint_rules["relations"] = [
             {
-                "charm": "nrpe",
-                "check": [["nrpe-host:local-monitors", "ubuntu:certificates"]],
+                "charm": "keystone",
+                "check": [["keystone:certificates", "ubuntu:certificates"]],
             }
         ]
         linter.check_relations(input_files[input_file_type])
@@ -1796,7 +1796,7 @@ applications:
                 "id": "missing-relations",
                 "tags": ["relation", "missing"],
                 "message": "Endpoint '{}' is missing relations with: {}".format(
-                    "nrpe:local-monitors", ["ubuntu"]
+                    "keystone:certificates", ["ubuntu"]
                 ),
             }
         )
@@ -1806,7 +1806,7 @@ applications:
         """Ensure that check_relation handle not exist error."""
         mock_message_handler = mocker.patch("jujulint.lint.Linter.message_handler")
         not_exist_relation = [
-            "nrpe-host:nrpe-external-master",
+            "nrpe:nrpe-external-master",
             "elasticsearch:nrpe-external-master",
         ]
         # add a relation rule that happen in the model
diff --git a/tests/unit/test_relations.py b/tests/unit/test_relations.py
index 121fe9e..1b2744c 100644
--- a/tests/unit/test_relations.py
+++ b/tests/unit/test_relations.py
@@ -11,29 +11,35 @@ RELATIONS = [["*:nrpe-external-master", "nrpe:nrpe-external-master"]]
 
 
 @pytest.mark.parametrize(
-    "correct_relation, input_file_type",
+    "correct_relation, input_file_type, expected_output",
     [
-        (RELATIONS, "juju-status"),
-        (RELATIONS, "juju-bundle"),
+        (RELATIONS, "juju-status", ["elasticsearch", "keystone"]),
+        (RELATIONS, "juju-bundle", ["elasticsearch", "keystone"]),
         (
             [["nrpe:nrpe-external-master", "*:nrpe-external-master"]],
             "juju-status",
+            ["elasticsearch", "keystone"],
         ),  # inverting sequence doesn't change the endpoint
         (
             [["nrpe:nrpe-external-master", "*:nrpe-external-master"]],
             "juju-bundle",
+            ["elasticsearch", "keystone"],
         ),  # inverting sequence doesn't change the endpoint
         (
             [["nrpe:nrpe-external-master", "keystone:nrpe-external-master"]],
             "juju-status",
+            ["keystone"],
         ),  # able to find specific app relation
         (
             [["nrpe:nrpe-external-master", "keystone:nrpe-external-master"]],
             "juju-bundle",
+            ["keystone"],
         ),  # able to find specific app relation
     ],
 )
-def test_relation_rule_valid(correct_relation, input_file_type, input_files):
+def test_relation_rule_valid(
+    correct_relation, input_file_type, expected_output, input_files
+):
     """Missing rules have empty set for endpoints with expected relations."""
     relation_rule = relations.RelationRule(
         input_file=input_files[input_file_type],
@@ -44,7 +50,9 @@ def test_relation_rule_valid(correct_relation, input_file_type, input_files):
         ubiquitous=True,
     )
     relation_rule.check()
-    assert relation_rule.missing_relations == {"nrpe:nrpe-external-master": list()}
+    assert relation_rule.missing_relations == {
+        "nrpe:nrpe-external-master": expected_output
+    }
     assert relation_rule.not_exist_error == list()
     assert relation_rule.missing_machines == list()
     assert relation_rule.__repr__() == "RelationRule(nrpe -> nrpe-external-master)"
@@ -144,8 +152,16 @@ def test_relation_not_exist_raise(input_file_type, input_files):
 @pytest.mark.parametrize(
     "expected_missing, exception, input_file_type",
     [
-        ({"nrpe:nrpe-external-master": ["foo-charm"]}, set(), "juju-status"),
-        ({"nrpe:nrpe-external-master": list()}, {"foo-charm"}, "juju-bundle"),
+        (
+            {"nrpe:nrpe-external-master": ["elasticsearch", "foo-charm", "keystone"]},
+            set(),
+            "juju-status",
+        ),
+        (
+            {"nrpe:nrpe-external-master": ["elasticsearch", "keystone"]},
+            {"foo-charm"},
+            "juju-bundle",
+        ),
     ],
 )
 def test_missing_relation_and_exception(