fix issues found by code review

Author: richm

library/network_connections.py

@@ -777,10 +777,7 @@ def connection_compare(
         if compare_flags is None:
             compare_flags = NM.SettingCompareFlags.IGNORE_TIMESTAMP
 
-        # this is really odd, but I'm assuming there is a double-negative
-        # here for a reason . . . .
-        # pylint: disable=unneeded-not
-        return not (not (con_a.compare(con_b, compare_flags)))
+        return con_a.compare(con_b, compare_flags)
 
     def connection_is_active(self, con):
         NM = Util.NM()
@@ -1625,10 +1622,7 @@ def connections_data(self):
                 raise AssertionError(
                     "invalid value {0} for self.check_mode".format(self.check_mode)
                 )
-            c = []
-            # pylint: disable=blacklisted-name
-            for _ in range(0, len(self.connections)):
-                c.append({"changed": False})
+            c = [{"changed": False}] * len(self.connections)
             self._connections_data = c
         return c
 
@@ -2015,8 +2009,7 @@ def _check_ethtool_setting_support(self, idx, connection):
                     idx, "ethtool.%s specified but not supported by NM", specified
                 )
 
-            # pylint: disable=blacklisted-name
-            for option, _ in specified.items():
+            for option in specified.keys():
                 nm_name = nm_get_name_fcnt(option)
                 if not nm_name:
                     self.log_fatal(

module_utils/network_lsr/ethtool.py

@@ -50,7 +50,8 @@ def get_perm_addr(ifname):
             res = ecmd.tobytes()
         except AttributeError:  # tobytes() is not available in python2
             res = ecmd.tostring()
-        # pylint: disable=blacklisted-name
+        # disable check for bad names:
+        # pylint: disable=C0102
         _, size, perm_addr = struct.unpack("II%is" % MAX_ADDR_LEN, res)
         perm_addr = Util.mac_ntoa(perm_addr[:size])
     except IOError:

module_utils/network_lsr/nm/active_connection.py

@@ -25,21 +25,15 @@ def deactivate_active_connection(nm_ac, timeout, check_mode):
         return False
     if not check_mode:
         main_loop = client.get_mainloop(timeout)
-        # pylint: disable=logging-format-interpolation
-        logging.debug(
-            "Deactivating {id} with timeout {timeout}".format(
-                id=nm_ac.get_id(), timeout=timeout
-            )
-        )
+        logging.debug("Deactivating %s with timeout %s", nm_ac.get_id(), timeout)
         user_data = main_loop
         handler_id = nm_ac.connect(
             NM_AC_STATE_CHANGED_SIGNAL, _nm_ac_state_change_callback, user_data
         )
-        # pylint: disable=logging-format-interpolation
         logging.debug(
-            "Registered {signal} on client.NM.ActiveConnection {id}".format(
-                signal=NM_AC_STATE_CHANGED_SIGNAL, id=nm_ac.get_id()
-            )
+            "Registered %s on client.NM.ActiveConnection %s",
+            NM_AC_STATE_CHANGED_SIGNAL,
+            nm_ac.get_id(),
         )
         if nm_ac.props.state != client.NM.ActiveConnectionState.DEACTIVATING:
             nm_client = client.get_client()
@@ -50,10 +44,7 @@ def deactivate_active_connection(nm_ac, timeout, check_mode):
                 _nm_ac_deactivate_call_back,
                 user_data,
             )
-            # pylint: disable=logging-format-interpolation
-            logging.debug(
-                "Deactivating client.NM.ActiveConnection {0}".format(nm_ac.get_id())
-            )
+            logging.debug("Deactivating client.NM.ActiveConnection %s", nm_ac.get_id())
         main_loop.run()
     return True
 
@@ -62,17 +53,14 @@ def _nm_ac_state_change_callback(nm_ac, state, reason, user_data):
     main_loop = user_data
     if main_loop.is_cancelled:
         return
-    # pylint: disable=logging-format-interpolation
     logging.debug(
-        "Got client.NM.ActiveConnection state change: {id}: {state} {reason}".format(
-            id=nm_ac.get_id(), state=state, reason=reason
-        )
+        "Got client.NM.ActiveConnection state change: %s: %s %s",
+        nm_ac.get_id(),
+        state,
+        reason,
     )
     if nm_ac.props.state == client.NM.ActiveConnectionState.DEACTIVATED:
-        # pylint: disable=logging-format-interpolation
-        logging.debug(
-            "client.NM.ActiveConnection {0} is deactivated".format(nm_ac.get_id())
-        )
+        logging.debug("client.NM.ActiveConnection %s is deactivated", nm_ac.get_id())
         main_loop.quit()
 
 
@@ -90,11 +78,8 @@ def _nm_ac_deactivate_call_back(nm_client, result, user_data):
         if e.matches(
             client.NM.ManagerError.quark(), client.NM.ManagerError.CONNECTIONNOTACTIVE
         ):
-            # pylint: disable=logging-format-interpolation
             logging.info(
-                "Connection is not active on {0}, no need to deactivate".format(
-                    nm_ac_id
-                )
+                "Connection is not active on %s, no need to deactivate", nm_ac_id
             )
             if nm_ac:
                 nm_ac.handler_disconnect(handler_id)

module_utils/network_lsr/nm/connection.py

@@ -29,11 +29,11 @@ def delete_remote_connection(nm_profile, timeout, check_mode):
             _nm_profile_delete_call_back,
             user_data,
         )
-        # pylint: disable=logging-format-interpolation
         logging.debug(
-            "Deleting profile {id}/{uuid} with timeout {timeout}".format(
-                id=nm_profile.get_id(), uuid=nm_profile.get_uuid(), timeout=timeout
-            )
+            "Deleting profile %s/%s with timeout %s",
+            nm_profile.get_id(),
+            nm_profile.get_uuid(),
+            timeout,
         )
         main_loop.run()
     return True
@@ -82,11 +82,11 @@ def volatilize_remote_connection(nm_profile, timeout, check_mode):
             _nm_profile_volatile_update2_call_back,
             user_data,
         )
-        # pylint: disable=logging-format-interpolation
         logging.debug(
-            "Volatilizing profile {id}/{uuid} with timeout {timeout}".format(
-                id=nm_profile.get_id(), uuid=nm_profile.get_uuid(), timeout=timeout
-            )
+            "Volatilizing profile %s/%s with timeout %s",
+            nm_profile.get_id(),
+            nm_profile.get_uuid(),
+            timeout,
         )
         main_loop.run()
     return True

module_utils/network_lsr/nm/provider.py

@@ -29,8 +29,7 @@ def deactivate_connection(self, connection_name, timeout, check_mode):
                     nm_ac, timeout, check_mode
                 )
         if not changed:
-            # pylint: disable=logging-format-interpolation
-            logging.info("No active connection for {0}".format(connection_name))
+            logging.info("No active connection for %s", connection_name)
 
         return changed
 
@@ -54,8 +53,7 @@ def volatilize_connection_by_uuid(self, uuid, timeout, check_mode):
                         nm_profile, timeout, check_mode
                     )
         if not changed:
-            # pylint: disable=logging-format-interpolation
-            logging.info("No connection with UUID {0} to volatilize".format(uuid))
+            logging.info("No connection with UUID %s to volatilize", uuid)
 
         return changed
 

tests/ensure_provider_tests.py

@@ -118,9 +118,7 @@ def create_nm_playbook(test_playbook):
         EXTRA_RUN_CONDITION, ""
     )
     if extra_run_condition:
-        extra_run_condition = "{0}{1}\n".format(
-            EXTRA_RUN_CONDITION_PREFIX, extra_run_condition
-        )
+        extra_run_condition = f"{EXTRA_RUN_CONDITION_PREFIX}{extra_run_condition}\n"
 
     nm_version_check = ""
     if minimum_nm_version:
@@ -213,7 +211,7 @@ def main():
 
     if missing:
         print("ERROR: No NM or initscripts tests found for:\n" + ", \n".join(missing))
-        print("Try to generate them with '{0} generate'".format(sys.argv[0]))
+        print(f"Try to generate them with '{sys.argv[0]} generate'")
         returncode = 1
 
     return returncode

tests/get_coverage.sh

@@ -31,7 +31,10 @@ call_ansible() {
 }
 
 remote_coverage_dir="$(mktemp -d /tmp/remote_coverage-XXXXXX)"
-trap 'rm -rf "${remote_coverage_dir}"' EXIT
+# we want to expand ${remote_coverage_dir} here, so tell SC to be quiet
+# https://github.com/koalaman/shellcheck/wiki/SC2064
+# shellcheck disable=SC2064
+trap "rm -rf '${remote_coverage_dir}'" EXIT
 ansible-playbook -i "${host}", get_coverage.yml -e "test_playbook=${playbook} destdir=${remote_coverage_dir}"
 
 #COVERAGE_FILE=remote-coverage coverage combine remote-coverage/tests_*/*/root/.coverage

tests/integration/test_ethernet.py

@@ -24,12 +24,11 @@
 
 class PytestRunEnvironment(nc.RunEnvironment):
     def log(self, connections, idx, severity, msg, **kwargs):
-        # pylint: disable=logging-format-interpolation
         if severity == nc.LogLevel.ERROR:
-            logging.error("Error: {0}".format(connections[idx]))
+            logging.error("Error: %s", connections[idx])
             raise RuntimeError(msg)
         else:
-            logging.debug("Log: {0}".format(connections[idx]))
+            logging.debug("Log: %s", connections[idx])
 
     def run_command(self, argv, encoding=None):
         command = subprocess.Popen(

tests/merge_coverage.sh

@@ -23,7 +23,10 @@ export COVERAGE_FILE="${1}"
 shift
 
 tempdir="$(mktemp -d /tmp/coverage_merge-XXXXXX)"
-trap 'rm -rf "${tempdir}"' EXIT
+# we want to expand ${tempdir} here, so tell SC to be quiet
+# https://github.com/koalaman/shellcheck/wiki/SC2064
+# shellcheck disable=SC2064
+trap "rm -rf '${tempdir}'" EXIT
 
 cp --backup=numbered -- "${@}" "${tempdir}"
 # FIXME: Would not work if coverage files are not hidden but they are by