Update for netmiko 4.x (#308)

* Update kwargs in nxos_device.py to fix nautobot-nornir. * Update for netmiko 4.x * Pylint updates. * Update CI for pylint py version. * Fix exceptions * Fix NXOS timeout * Add refresh facts. * Remove save() from install_os as it causes error. * Update pyntc/devices/aireos_device.py Post-review commit (lint) Co-authored-by: Jeff Kala <[email protected]> --------- Co-authored-by: Jeff Kala <[email protected]>
networktocode · Dec 22, 2023 · 2a6ea1d · 2a6ea1d
1 parent 26c2369
commit 2a6ea1d
Show file tree

Hide file tree

Showing 18 changed files with 186 additions and 330 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -83,7 +83,7 @@ jobs:
     strategy:
       fail-fast: true
       matrix:
-        python-version: ["3.8", "3.9", "3.10", "3.11"]  # "3.10" add back after pyeapi new release.
+        python-version: ["3.8", "3.11"]
     runs-on: "ubuntu-20.04"
     env:
       PYTHON_VER: "${{ matrix.python-version }}"
@@ -114,49 +114,48 @@ jobs:
       - "pydocstyle"
       - "flake8"
       - "yamllint"
-  # TODO: Re-enable after initial pylint issue is completed. https://github.com/networktocode/pyntc/issues/249
-  # pylint:
-  #   runs-on: "ubuntu-20.04"
-  #   strategy:
-  #     fail-fast: true
-  #     matrix:
-  #       python-version: ["3.7"]
-  #   env:
-  #     PYTHON_VER: "${{ matrix.python-version }}"
-  #   steps:
-  #     - name: "Check out repository code"
-  #       uses: "actions/checkout@v2"
-  #     - name: "Setup environment"
-  #       uses: "networktocode/gh-action-setup-poetry-environment@v2"
-  #     - name: "Get image version"
-  #       run: "echo IMAGE_VER=`poetry version -s`-py${{ matrix.python-version }} >> $GITHUB_ENV"
-  #     - name: "Set up Docker Buildx"
-  #       id: "buildx"
-  #       uses: "docker/setup-buildx-action@v1"
-  #     - name: "Load the image from cache"
-  #       uses: "docker/build-push-action@v2"
-  #       with:
-  #         builder: "${{ steps.buildx.outputs.name }}"
-  #         context: "./"
-  #         push: false
-  #         load: true
-  #         tags: "${{ env.IMAGE_NAME }}:${{ env.IMAGE_VER }}"
-  #         file: "./Dockerfile"
-  #         cache-from: "type=gha,scope=${{ env.IMAGE_NAME }}-${{ env.IMAGE_VER }}-py${{ matrix.python-version }}"
-  #         cache-to: "type=gha,scope=${{ env.IMAGE_NAME }}-${{ env.IMAGE_VER }}-py${{ matrix.python-version }}"
-  #         build-args: |
-  #           PYTHON_VER=${{ env.PYTHON_VER }}
-  #     - name: "Debug: Show docker images"
-  #       run: "docker image ls"
-  #     - name: "Linting: Pylint"
-  #       run: "poetry run invoke pylint"
-  #   needs:
-  #     - "build"
+  pylint:
+    runs-on: "ubuntu-20.04"
+    strategy:
+      fail-fast: true
+      matrix:
+        python-version: ["3.8"]
+    env:
+      PYTHON_VER: "${{ matrix.python-version }}"
+    steps:
+      - name: "Check out repository code"
+        uses: "actions/checkout@v2"
+      - name: "Setup environment"
+        uses: "networktocode/gh-action-setup-poetry-environment@v2"
+      - name: "Get image version"
+        run: "echo IMAGE_VER=`poetry version -s`-py${{ matrix.python-version }} >> $GITHUB_ENV"
+      - name: "Set up Docker Buildx"
+        id: "buildx"
+        uses: "docker/setup-buildx-action@v1"
+      - name: "Load the image from cache"
+        uses: "docker/build-push-action@v2"
+        with:
+          builder: "${{ steps.buildx.outputs.name }}"
+          context: "./"
+          push: false
+          load: true
+          tags: "${{ env.IMAGE_NAME }}:${{ env.IMAGE_VER }}"
+          file: "./Dockerfile"
+          cache-from: "type=gha,scope=${{ env.IMAGE_NAME }}-${{ env.IMAGE_VER }}-py${{ matrix.python-version }}"
+          cache-to: "type=gha,scope=${{ env.IMAGE_NAME }}-${{ env.IMAGE_VER }}-py${{ matrix.python-version }}"
+          build-args: |
+            PYTHON_VER=${{ env.PYTHON_VER }}
+      - name: "Debug: Show docker images"
+        run: "docker image ls"
+      - name: "Linting: Pylint"
+        run: "poetry run invoke pylint"
+    needs:
+      - "build"
   pytest:
     strategy:
       fail-fast: true
       matrix:
-        python-version: ["3.8", "3.9", "3.10", "3.11"]
+        python-version: ["3.8", "3.11"]
     runs-on: "ubuntu-20.04"
     env:
       PYTHON_VER: "${{ matrix.python-version }}"
@@ -188,8 +187,7 @@ jobs:
       - name: "Run Tests"
         run: "poetry run invoke pytest"
     needs:
-      # Remove everything but pylint once pylint is passing.
-      # - "pylint"
+      - "pylint"
       - "bandit"
       - "pydocstyle"
       - "flake8"

diff --git a/poetry.lock b/poetry.lock
diff --git a/pyntc/devices/aireos_device.py b/pyntc/devices/aireos_device.py
@@ -87,9 +87,7 @@ def __init__(  # nosec  # pylint: disable=too-many-arguments
         self.native = None
         self.secret = secret
         self.port = int(port) if port else 22
-        self.delay_factor_compat = kwargs.get("delay_factor_compat", True)
-        self.global_delay_factor = kwargs.get("global_delay_factor", 1)
-        self.delay_factor = kwargs.get("delay_factor", 1)
+        self.read_timeout_override = kwargs.get("read_timeout_override")
         self._connected = False
         self.open(confirm_active=confirm_active)
         log.init(host=host)
@@ -588,7 +586,7 @@ def config(self, command, **netmiko_args):
             raise TypeError(f"Netmiko Driver's {err.args[0]}")
         # TODO: Remove this when deprecating config_list method
         except CommandError as err:
-            if not original_command_is_str:
+            if not original_command_is_str:  # pylint: disable=no-else-raise
                 log.error(
                     "Host %s: Commands %s returned the error %s",
                     self.host,
@@ -862,7 +860,7 @@ def file_copy(
         filepath,
         protocol="sftp",
         filetype="code",
-        delay_factor=10,
+        read_timeout=1000,
     ):
         """
         Copy a file from server to Controller.
@@ -874,7 +872,7 @@ def file_copy(
             filepath (str): The full path to the file on the ``server``.
             protocol (str): The transfer protocol to use to transfer the file.
             filetype (str): The type of file per aireos definitions.
-            delay_factor (int): The Netmiko delay factor to wait for device to complete transfer.
+            read_timeout (int): The Netmiko read_timeout to wait for device to complete transfer.
 
         Returns:
             bool: True when the file was transferred, False when the file is deemed to already be on the device.
@@ -929,7 +927,7 @@ def file_copy(
         try:
             response = self.native.send_command_timing("transfer download start")
             if "Are you sure you want to start? (y/N)" in response:
-                response = self.show("y", auto_find_prompt=False, delay_factor=delay_factor)
+                response = self.show("y", auto_find_prompt=False, read_timeout=read_timeout)
         except CommandError as error:
             log.error(
                 "Host %s: File transfer error %s\n\n%s",
@@ -1097,8 +1095,7 @@ def open(self, confirm_active=True):
                 username=self.username,
                 password=self.password,
                 port=self.port,
-                delay_factor_compat=self.delay_factor_compat,
-                global_delay_factor=self.global_delay_factor,
+                read_timeout_override=self.read_timeout_override,
                 secret=self.secret,
                 verbose=False,
             )
@@ -1182,7 +1179,7 @@ def reboot(self, wait_for_reload=False, controller="self", save_config=True, **k
             if wait_for_reload:
                 time.sleep(10)
                 self._wait_for_device_reboot()
-        except Exception as err:
+        except Exception as err:  # pylint: disable=broad-exception-caught
             log.error(err)
             log.error(err.__class__)
 
@@ -1364,7 +1361,7 @@ def show(self, command, expect_string=None, **netmiko_args):
             raise TypeError(f"Netmiko Driver's {err.args[0]}")
         # TODO: Remove this when deprecating config_list method
         except CommandError as err:
-            if not original_command_is_str:
+            if not original_command_is_str:  # pylint: disable=no-else-raise
                 log.error(
                     "Host %s: Command error for commands %s with message %s.",
                     self.host,

diff --git a/pyntc/devices/asa_device.py b/pyntc/devices/asa_device.py
@@ -55,9 +55,7 @@ def __init__(self, host: str, username: str, password: str, secret="", port=None
         self.secret = secret
         self.port = int(port) if port else 22
         self.kwargs = kwargs
-        self.delay_factor_compat = kwargs.get("delay_factor_compat", True)
-        self.global_delay_factor: int = kwargs.get("global_delay_factor", 1)
-        self.delay_factor: int = kwargs.get("delay_factor", 1)
+        self.read_timeout_override = kwargs.get("read_timeout_override")
         self._connected = False
         self.open()
         self._peer_device: Optional[ASADevice] = None
@@ -262,7 +260,7 @@ def _show_vlan(self):
         log.debug("Host %s: Successfully executed command 'show vlan' with responses %s.", self.host, show_vlan_out)
         return show_vlan_out.split(",")
 
-    def _uptime_components(self, uptime_full_string):  # pylint: disable=no-self-use
+    def _uptime_components(self, uptime_full_string):
         match_days = re.search(r"(\d+) days?", uptime_full_string)
         match_hours = re.search(r"(\d+) hours?", uptime_full_string)
         match_minutes = re.search(r"(\d+) mins?", uptime_full_string)
@@ -376,8 +374,8 @@ def boot_options(self):
         else:
             boot_image = None
 
-        log.debug("Host %s: the boot options are %s", self.host, dict(sys=boot_image))
-        return dict(sys=boot_image)
+        log.debug("Host %s: the boot options are %s", self.host, {"sys": boot_image})
+        return {"sys": boot_image}
 
     def checkpoint(self, checkpoint_file):
         """
@@ -602,7 +600,7 @@ def install_os(self, image_name, **vendor_specifics):
             self._wait_for_device_reboot(timeout=timeout)
             if not self._image_booted(image_name):
                 log.error("Host %s: OS install error for image %s", self.host, image_name)
-                raise OSInstallError(hostname=self.facts.get("hostname"), desired_boot=image_name)
+                raise OSInstallError(hostname=self.hostname, desired_boot=image_name)
 
             log.info("Host %s: OS image %s installed successfully.", self.host, image_name)
             return True
@@ -728,8 +726,7 @@ def open(self):
                 username=self.username,
                 password=self.password,
                 port=self.port,
-                delay_factor_compat=self.delay_factor_compat,
-                global_delay_factor=self.global_delay_factor,
+                read_timeout_override=self.read_timeout_override,
                 secret=self.secret,
                 verbose=False,
             )
@@ -890,7 +887,7 @@ def reboot(self, wait_for_reload=False, **kwargs):
             if wait_for_reload:
                 time.sleep(10)
                 self._wait_for_device_reboot()
-        except Exception as err:
+        except Exception as err:  # pylint: disable=broad-exception-caught
             log.error(err)
             log.error(err.__class__)
 

diff --git a/pyntc/devices/eos_device.py b/pyntc/devices/eos_device.py
@@ -122,13 +122,13 @@ def _interfaces_status_list(self):
         log.debug("Host %s: interfaces detailed list %s.", self.host, interface_status_list)
         return interface_status_list
 
-    def _parse_response(self, response, raw_text):  # pylint: disable=no-self-use
+    def _parse_response(self, response, raw_text):
         if raw_text:
             return list(x["result"]["output"] for x in response)
 
         return list(x["result"] for x in response)
 
-    def _uptime_to_string(self, uptime):  # pylint: disable=no-self-use
+    def _uptime_to_string(self, uptime):
         days = uptime / (24 * 60 * 60)
         uptime = uptime % (24 * 60 * 60)
 
@@ -176,8 +176,8 @@ def boot_options(self):
         """
         image = self.show("show boot-config")["softwareImage"]
         image = image.replace("flash:", "")
-        log.debug("Host %s: the boot options are %s", self.host, dict(sys=image))
-        return dict(sys=image)
+        log.debug("Host %s: the boot options are %s", self.host, {"sys": image})
+        return {"sys": image}
 
     def checkpoint(self, checkpoint_file):
         """Copy running config checkpoint.

diff --git a/pyntc/devices/f5_device.py b/pyntc/devices/f5_device.py
@@ -49,12 +49,11 @@ def _check_free_space(self, min_space=0):
             raise ValueError("Could not get free space")
 
         if free_space >= min_space:
+            log.debug("Host %s: Free space %s is sufficient.", self.host, free_space)
             return
-        elif free_space < min_space:
-            log.error("Host %s: Not enough free space for min space requirement %s.", self.host, min_space)
-            raise NotEnoughFreeSpaceError(hostname=self.host, min_space=min_space)
 
-        log.debug("Host %s: Free space %s is sufficient.", self.host, free_space)
+        log.error("Host %s: Not enough free space for min space requirement %s.", self.host, min_space)
+        raise NotEnoughFreeSpaceError(hostname=self.host, min_space=str(min_space))
 
     def _check_md5sum(self, filename, checksum):
         """Check is md5sum is correct.
@@ -71,9 +70,9 @@ def _check_md5sum(self, filename, checksum):
         if checksum == md5sum:
             log.debug("Host %s: Checksums match.", self.host)
             return True
-        else:
-            log.debug("Host %s: Checksums do not match.", self.host)
-            return False
+
+        log.debug("Host %s: Checksums do not match.", self.host)
+        return False
 
     @staticmethod
     def _file_copy_local_file_exists(filepath):
@@ -191,7 +190,7 @@ def _image_booted(self, image_name, **vendor_specifics):
         """
         volume = vendor_specifics.get("volume")
         log.debug("Host %s: Checking if image %s has been booted.", self.host, image_name)
-        return True if self._get_active_volume() == volume else False
+        return self._get_active_volume() == volume
 
     def _image_exists(self, image_name):
         """Check if image exists on the device.
@@ -212,9 +211,9 @@ def _image_exists(self, image_name):
         if image_name in all_images:
             log.debug("Host %s: Image %s exists.", self.host, image_name)
             return True
-        else:
-            log.debug("Host %s: Image %s does not exist.", self.host, image_name)
-            return False
+
+        log.debug("Host %s: Image %s does not exist.", self.host, image_name)
+        return False
 
     def _image_install(self, image_name, volume):
         """Request installation of the image on a volume.
@@ -298,6 +297,8 @@ def _upload_image(self, image_filepath):
                     end = size
                 content_range = f"{start}-{end - 1}/{size}"
                 headers["Content-Range"] = content_range
+                # pylint: disable=missing-timeout
+                # TODO Add timeout to requests.post, missing timeout can cause the method to hang indefinitely
                 requests.post(
                     upload_uri,
                     auth=(self.username, self.password),
@@ -328,7 +329,7 @@ def _uptime_to_string(uptime):
         uptime = uptime % 60
         seconds = uptime
 
-        return "%02d:%02d:%02d:%02d" % (days, hours, mins, seconds)
+        return "%02d:%02d:%02d:%02d" % (days, hours, mins, seconds)  # pylint: disable=consider-using-f-string
 
     def _volume_exists(self, volume_name):
         """Check if volume exist.
@@ -368,7 +369,6 @@ def _wait_for_device_reboot(self, volume_name, timeout=600):
                 log.debug("Host %s: Reboot successfull.", self.host)
             except Exception:  # noqa E722 # nosec  # pylint: disable=broad-except
                 log.error("Host %s: Error while rebooting.", self.host)
-                pass
         log.debug("Host %s: Reboot not successfull.", self.host)
         return False
 
@@ -607,9 +607,8 @@ def file_copy_remote_exists(self, src, dest=None, **kwargs):
         if not self._image_match(image_name=file_basename, checksum=local_md5sum):
             log.debug("Host %s: File %s does not already exist on remote.", self.host, src)
             return False
-        else:
-            log.debug("Host %s: File %s already exists on remote.", self.host)
-            return True
+        log.debug("Host %s: File %s already exists on remote.", self.host)
+        return True
 
     def image_installed(self, image_name, volume):
         """Check if image is installed on specified volume.