Address comments from code review

- removes printing in test
- rewrites line using with context manager
- changes variable name
- passes cp as an argument to the fucntion
- shortens long lines

ssm/agents.py

@@ -299,7 +299,7 @@ def run_receiver(protocol, brokers, project, token, cp, log, dn_file):
         ssm.set_dns(dns)
 
         log.info('Fetching banned DNs.')
-        banned_dns = get_banned_dns(log)
+        banned_dns = get_banned_dns(log, cp)
         ssm.set_banned_dns(banned_dns)
 
     except Exception as e:
@@ -385,28 +385,29 @@ def get_dns(dn_file, log):
     return dns
 
 
-def get_banned_dns(log):
+def get_banned_dns(log, cp):
     """Retrieve the list of banned dns"""
-    banned_dns_list = []
+    banned_dns = []
     try:
         banned_dns_path = cp.get('auth', 'banned-dns')
-        banned_dns_file = os.path.normpath(os.path.expandvars(banned_dns_path))
+        banned_dns_file = os.path.normpath(
+            os.path.expandvars(banned_dns_path))
     except ConfigParser.NoOptionError:
         banned_dns_file = None
     f = None
     try:
-        f = open(banned_dns_file, 'r')
-        lines = f.readlines()
-        for line in lines:
-            if line.isspace() or line.strip().startswith('#'):
-                continue
-            elif line.strip().startswith('/'):
-                banned_dns_list.append(line.strip())
-            else:
-                log.warning('DN in banned dns list is not in correct format: %s', line)
+        with open(banned_dns_file, 'r') as f:
+            lines = f.readlines()
+            for line in lines:
+                if line.isspace() or line.strip().startswith('#'):
+                    continue
+                elif line.strip().startswith('/'):
+                    banned_dns.append(line.strip())
+                else:
+                    log.warning('DN in banned dns list is not in '
+                                'the correct format: %s', line)
     finally:
         if f is not None:
             f.close()
 
-    return banned_dns_list
-
+    return banned_dns

test/test_ssm.py

@@ -233,8 +233,6 @@ def test_dns_saved_to_queue(self, mock_handle_msg):
         for dn in valid_dns:
             # Capture the log output so we can use it in assertions
             with LogCapture() as log:
-                print("Testing dn:", dn)
-
                 message_valid = """APEL-summary-job-message: v0.2
                         Site: RAL-LCG2
                         Month: 3
@@ -260,16 +258,12 @@ def test_dns_saved_to_queue(self, mock_handle_msg):
 
                 self.assertIn('Message saved to incoming queue', str(log))
 
-                print("Test Passed.\n")
-
         # As there are several different ways messages can be rejected,
         # Keep a count to test a different method for each dn
         dnCount = 1
         for dn in rejected_dns:
             # Capture the log output so we can use it in assertions
             with LogCapture() as log:
-                print("Testing dn:", dn)
-
                 message_rejected = """APEL-summary-job-message: v0.2
                         Site: RAL-LCG2
                         Month: 3
@@ -304,8 +298,6 @@ def test_dns_saved_to_queue(self, mock_handle_msg):
 
                 self.assertIn('Message saved to reject queue', str(log))
 
-                print("Test Passed.\n")
-
                 dnCount = dnCount + 1
 
         # For each dn in the banned dns list,
@@ -314,8 +306,6 @@ def test_dns_saved_to_queue(self, mock_handle_msg):
         for dn in banned_dns:
             # Capture the log output so we can use it in assertions
             with LogCapture() as log:
-                print("Testing dn:", dn)
-
                 message_banned = """APEL-summary-job-message: v0.2
                         Site: RAL-LCG2
                         Month: 3
@@ -341,8 +331,6 @@ def test_dns_saved_to_queue(self, mock_handle_msg):
 
                 self.assertIn('Message dropped as was sent from a banned dn', str(log))
 
-                print("Test Passed.\n")
-
 
 TEST_KEY_FILE = '/tmp/test.key'