Merge ~martin-kalcok/juju-lint:warn_on_missing_binding into juju-lint:master

[Martin Kalcok]

Add warnings if bundle does not specify explicit bindings

Added warnings if bundle does not contain explicit bindings for application endpoints.

3 types of warnings added:

* When there are relationships but no application specifies bindings
* When some application specify bindings but others don't
* when application specify bindings but do not include binding for default endpoint ("")

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

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

[Gabriel Cocenza (gabrielcocenza)]

Hi Martin.
I've left some comments for you in the code.

review: Needs Fixing

[Gabriel Cocenza (gabrielcocenza)]

I left two comments about nit things. I recommend rebase the master branch that is already fixing test_check_spaces_detect_mismatches. Otherwise, LGTM.

review: Needs Fixing

[Gabriel Cocenza (gabrielcocenza)]

LGTM.

review: Approve

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

Change successfully merged at revision e5d34a5b3d630954cfaeed061591ebc7f358d5d1

diff --git a/jujulint/check_spaces.py b/jujulint/check_spaces.py
index 17b8d8c..337f9b2 100644
--- a/jujulint/check_spaces.py
+++ b/jujulint/check_spaces.py
@@ -1,5 +1,8 @@
 #!/usr/bin/python3
 """Checks for space mismatches between relation endpoints."""
+from logging import getLogger
+
+LOGGER = getLogger(__name__)
 
 
 class Relation:
@@ -116,8 +119,13 @@ def get_application_spaces(application_list, parsed_yaml):
     """Return a dictionary with app.binding=space mappings."""
     app_spaces = {}
     for app in application_list:
+        bindings = parsed_yaml["applications"][app].get('bindings', {})
+        if not bindings:
+            LOGGER.warning("Application %s is missing explicit bindings", app)
+            continue
+        if not bindings.get(""):
+            LOGGER.warning("Application %s does not define explicit default binding", app)
         app_spaces[app] = {}
-        bindings = parsed_yaml["applications"][app]["bindings"]
         for name, value in bindings.items():
             app_spaces[app][name] = value
     return app_spaces
diff --git a/jujulint/lint.py b/jujulint/lint.py
index e908b7a..748fc8a 100755
--- a/jujulint/lint.py
+++ b/jujulint/lint.py
@@ -1106,13 +1106,22 @@ class Linter:
             if "relations" in parsed_yaml:
                 # "bindings" *should* be in exported bundles, *unless* no custom bindings exist,
                 # in which case "juju export-bundle" omits them. See LP#1949883.
-                if "bindings" in list(parsed_yaml[applications].values())[0]:
+                bindings = any(
+                    "bindings" in app for app in parsed_yaml[applications].values()
+                )
+                if bindings:
+                    # try:
                     self.check_spaces(parsed_yaml)
-
+                    # except Exception as e:
+                    #     self._log_with_header(
+                    #         "Encountered error while checking spaces: {}".format(e),
+                    #         level=logging.WARN
+                    #     )
                 else:
                     self._log_with_header(
-                        "Relations detected but custom bindings not found; "
-                        "skipping space binding checks."
+                        "Relations detected but explicit bindings not found; "
+                        "Not specifying explicit bindings may cause problems on models"
+                        " with multiple network spaces.", level=logging.WARNING
                     )
             else:
                 self._log_with_header(
diff --git a/tests/test_jujulint.py b/tests/test_jujulint.py
index cff1d6b..11cc514 100644
--- a/tests/test_jujulint.py
+++ b/tests/test_jujulint.py
@@ -6,6 +6,8 @@ from unittest import mock
 
 import pytest
 
+from jujulint import check_spaces
+
 
 class TestUtils:
     """Test the jujulint utilities."""
@@ -820,3 +822,71 @@ applications:
         errors = linter.output_collector["errors"]
         assert len(errors) == 0
         assert mock_log.call_count == 0
+
+    def test_check_spaces_missing_explicit_bindings(self, linter, mocker):
+        """Test that check_spaces shows warning if some application are missing bindings
+
+        This warning should be triggerred if some applications have bindings and some
+        dont.
+        """
+        logger_mock = mocker.patch.object(check_spaces, "LOGGER")
+
+        app_without_binding = "prometheus-app"
+        bundle = {
+            "applications": {
+                app_without_binding: {},
+                "telegraf-app": {
+                    "bindings": {
+                        "": "alpha",
+                        "prometheus-client": "alpha",
+                    },
+                },
+            },
+            "relations": [
+                ["telegraf-app:prometheus-client", "prometheus-app:target"],
+            ],
+        }
+
+        expected_warning = "Application %s is missing explicit bindings"
+
+        linter.check_spaces(bundle)
+
+        logger_mock.warning.assert_called_once_with(expected_warning,
+                                                    app_without_binding)
+
+    def test_check_spaces_missing_default_endpoint_binding(self, linter, mocker):
+        """Raise warning if application is missing explicit default binding.
+
+        Aside from specifying binding for each endpoint explicitly, bundle can also
+        specify default space (represented by empty string ""). Any endpoint that's not
+        mentioned explicitly will be bound to this default space.
+        Juju lint should raise warning if bundles do not define default space.
+        """
+        logger_mock = mocker.patch.object(check_spaces, "LOGGER")
+        app_without_default_space = "telegraf-app"
+
+        bundle = {
+            "applications": {
+                "prometheus-app": {
+                    "bindings": {
+                        "": "alpha",
+                        "target": "alpha",
+                    },
+                },
+                app_without_default_space: {
+                    "bindings": {
+                        "prometheus-client": "alpha",
+                    },
+                },
+            },
+            "relations": [
+                ["telegraf-app:prometheus-client", "prometheus-app:target"],
+            ],
+        }
+
+        expected_warning = "Application %s does not define explicit default binding"
+
+        linter.check_spaces(bundle)
+
+        logger_mock.warning.assert_called_once_with(expected_warning,
+                                                    app_without_default_space)