Avoid linting failures by unconditionally defining a logger property.

Recent changes to the linting checks have resulted in any changes in and
around configuration fail in CI with the complaint that logger is not
defined.

In trying to understand what was happening it was found that some amount
of confusion was occurring having both logger and logger_obj properties.
Attempt to fix this by:
1) unconditionally defining both properties
2) always setting both properties
3) determining the type of logger being assigned and set the internal
   properties as appropriate

Expanding on the latter, loggers are almost always set as assignment to
.logger, but this was not always being passed the same kind of object. At
times this was a bare logger (i.e. info(), .debug() etc) but sometimes it
was something with .reopen() which would then simply be thrown away and
thus reload() would not actually work. Fix this by detecting a .reopen()
method and correctly referencing such an object.

Author: albu-diku

mig/shared/configuration.py

@@ -37,6 +37,7 @@
 import copy
 import datetime
 import functools
+import inspect
 import os
 import pwd
 import re
@@ -67,7 +68,7 @@
         generic_valid_days, DEFAULT_USER_ID_FORMAT, valid_user_id_formats, \
         valid_filter_methods, default_twofactor_auth_apps, \
         mig_conf_section_dirname
-    from mig.shared.logger import Logger, SYSLOG_GDP
+    from mig.shared.logger import Logger, BareLoggerAdapter, SYSLOG_GDP
     from mig.shared.htmlgen import menu_items, vgrid_items
     from mig.shared.fileio import read_file, load_json, write_file
 except ImportError as ioe:
@@ -693,7 +694,6 @@ def get(self, *args, **kwargs):
     'logfile': '',
     'loglevel': '',
     'logger_obj': None,
-    'logger': None,
     'gdp_logger_obj': None,
     'gdp_logger': None,
     'auth_logger_obj': None,
@@ -757,6 +757,13 @@ def __init__(self, config_file, verbose=False, skip_log=False,
         # Explicitly init a few helpers hot-plugged and used in ways where it's
         # less obvious if they are always guaranteed to already be initialized.
         self.default_page = None
+
+        # internal state
+        self._loaded = False
+
+        # logging related
+        self.logger_obj = None
+        self._logger = None
         self.auth_logger_obj = None
         self.gdp_logger_obj = None
 
@@ -770,6 +777,34 @@ def __init__(self, config_file, verbose=False, skip_log=False,
                                disable_auth_log=disable_auth_log,
                                _config_file=config_file)
 
+    @property
+    def logger(self):
+        assert self._logger, "logging attempt prior to logger availability"
+        return self._logger
+
+    @logger.setter
+    def logger(self, logger):
+        """Setter method that correctly sets logger related properties."""
+
+        # attempt to determine what type of objetc we were given - this logic
+        # exists to deal with some fallout from having both logger_obj and
+        # logger properties and which of them should be set
+
+        if inspect.ismethod(getattr(logger, 'reopen', None)):
+            # we have a logger_obj, not a plain logger
+            # record it that way to ensure it could be corrctly reopened where
+            # otherwise refefences to the object that has it may be lost and it
+            # may not occur
+            self.logger_obj = logger
+            self._logger = logger.logger
+        elif inspect.ismethod(getattr(logger, 'info', None)):
+            # we have a bare logger object based on the sanity check
+            self._logger = logger
+            self.logger_obj = BareLoggerAdapter(logger)
+        else:
+            raise
+
+
     def reload_config(self, verbose, skip_log=False, disable_auth_log=False,
                       _config_file=None):
         """Re-read and parse configuration file. Optional skip_log arg
@@ -786,12 +821,6 @@ def reload_config(self, verbose, skip_log=False, disable_auth_log=False,
         _config_file = _config_file or self.config_file
         assert _config_file is not None
 
-        try:
-            if self.logger:
-                self.logger.info('reloading configuration and reopening log')
-        except:
-            pass
-
         try:
             config_file_is_path = os.path.isfile(_config_file)
         except TypeError:
@@ -841,13 +870,17 @@ def reload_config(self, verbose, skip_log=False, disable_auth_log=False,
 
         # reopen or initialize logger
 
-        if self.logger_obj:
+        if self._loaded:
+            self.logger.info('reloading configuration and reopening log')
             self.logger_obj.reopen()
         else:
-            self.logger_obj = Logger(self.loglevel, logfile=self.log_path)
+            self.logger = Logger(self.loglevel, logfile=self.log_path)
+            self.logger.info('loading configuration and opening log')
+
+            # record that the object has been populated
+            self._loaded = True
 
-        logger = self.logger_obj.logger
-        self.logger = logger
+        logger = self.logger
 
         # print "logger initialized (level " + logger_obj.loglevel() + ")"
         # logger.debug("logger initialized")

mig/shared/logger.py

@@ -67,6 +67,23 @@ def _name_to_format(name):
     return formats[name]
 
 
+class BareLoggerAdapter:
+    """Small wrapper to adapt an arbitrary bare logger to the MiG Logger API"""
+
+    def __init__(self, logger):
+        self._logger = logger
+
+    @property
+    def logger(self):
+        return self._logger
+
+    def reopen(self):
+        pass
+
+    def shutdown(self):
+        pass
+
+
 class SysLogLibHandler(logging.Handler):
     """A logging handler that emits messages to syslog.syslog."""
 

tests/fixture/mig_shared_configuration--new.json

@@ -104,8 +104,6 @@
   ],
   "log_dir": "",
   "logfile": "",
-  "logger": null,
-  "logger_obj": null,
   "loglevel": "",
   "lrmstypes": [],
   "mig_code_base": "",

tests/test_mig_shared_configuration.py

@@ -34,6 +34,7 @@
 from tests.support import MigTestCase, TEST_DATA_DIR, PY2, testmain, \
     fixturefile
 from mig.shared.configuration import Configuration
+from mig.shared.logger import null_logger
 
 
 def _is_method(value):
@@ -42,7 +43,7 @@ def _is_method(value):
 
 def _to_dict(obj):
     return {k: v for k, v in inspect.getmembers(obj)
-            if not (k.startswith('__') or _is_method(v))}
+            if not (k.startswith('_') or k.startswith('logger') or _is_method(v))}
 
 
 class MigSharedConfiguration(MigTestCase):
@@ -320,6 +321,9 @@ def test_default_object(self):
             'mig_shared_configuration--new', fixture_format='json')
 
         configuration = Configuration(None)
+        # attach a null logger to sidestep the useful logger before available
+        # assertion which would otherwise blow when the object is inspected
+        configuration.logger = null_logger("test_configuration")
         # TODO: the following work-around default values set for these on the
         #       instance that no longer make total sense but fiddling with them
         #       is better as a follow-up.