Conversation
smk4664
requested review from
jeffkala,
pke11y and
pszulczewski
as code owners
Ignore the import position as Ruff is handling that.
| if TYPE_CHECKING: | ||
| from netmiko.base_connection import BaseConnection | ||
|
|
||
| from pyntc import log # pylint: disable=wrong-import-position |
There was a problem hiding this comment.
Would it make pylint and isort happy if you moved the conditional import above down below all of the other imports?
There was a problem hiding this comment.
That sounds like it is going to be the solution I will test that.
| for _ in range(10): | ||
| if current_prompt in output: | ||
| return | ||
| # Assume that the filename and address are sent with the url. |
There was a problem hiding this comment.
| # Assume that the filename and address are sent with the url. | |
| # Accept defaults for these prompts because the filename and address are sent with the url. |
| # Use VRF specified, unless using http or https, which don't support vrf in the copy command | ||
| if self.source_file.vrf and self.source_file.scheme not in ["http", "https"]: | ||
| command = f"{command} vrf {self.source_file.vrf}" | ||
| output = self.ssh_ctl_chan.send_command(command, expect_string=expect_regex, read_timeout=300) |
There was a problem hiding this comment.
Is this going to fail if the file transfer takes longer than 5 minutes? While the file is transferring netmiko is just waiting to match on one of the prompts right? I think we're still going to see use cases where large files are transferred over legacy circuits that may take a long time.
There was a problem hiding this comment.
Fair, I didn't know what to set this to. I will increase and make this a variable.
| if self.source_file.vrf and self.source_file.scheme not in ["http", "https"]: | ||
| command = f"{command} vrf {self.source_file.vrf}" | ||
| output = self.ssh_ctl_chan.send_command(command, expect_string=expect_regex, read_timeout=300) | ||
| for _ in range(10): |
There was a problem hiding this comment.
I think this should be an endless loop or a while current_prompt not in output and we probably need to raise an exception if none of the prompts match?
| command = f"{command} vrf {self.source_file.vrf}" | ||
| output = self.ssh_ctl_chan.send_command(command, expect_string=expect_regex, read_timeout=300) | ||
| for _ in range(10): | ||
| if current_prompt in output: |
There was a problem hiding this comment.
There's a 1:1 mapping of prompts in this loop and prompts in expect_regex that's being passed to netmiko above but that's not clearly defined as a requirement. If someone adds another prompt to the regex above but forgets to add it here this would fall through and it would be difficult to troubleshoot. If netmiko doesn't already have a pattern for responding to prompts we should write our own here. eg:
prompt_answers = {
current_prompt: None, # break out of the loop
r"(confirm|Address or name of remote host|Source filename|Destination filename)": "", # Press enter
r"Password": self.source_file.token,
r"Source username": self.source_file.username,
r"yes/no|Are you sure you want to continue connecting": "yes",
}There was a problem hiding this comment.
We could build the expect_regex from '|'.join(prompt_answers.keys()) so that the two code paths are always in sync.
| dest_sha512 = self.process_md5(dest_sha512) # Process MD5 still does what we want for parsing the output. | ||
| return dest_sha512 | ||
|
|
||
| def compare_md5(self) -> bool: |
There was a problem hiding this comment.
I think we're trying too hard to fit a square peg into a round hole subclassing the CiscoIosFileTransfer class. I would prefer if we just vendored the whole thing and rip out the parts we don't need (get/put). That gives us more flexibility to update it as we see fit to support things like different hashing algorithms. I know I won't be able to remember all of the workarounds that were required to make this work when the time comes to add support for another platform.
| download_url (str): The URL to download the file from. Can include credentials, but it's recommended to use the username and token fields instead for security reasons. | ||
| checksum (str): The expected checksum of the file. | ||
| file_name (str): The name of the file to be saved on the device. | ||
| hashing_algorithm (str): The hashing algorithm to use for checksum verification. Defaults to "md5". |
There was a problem hiding this comment.
| hashing_algorithm (str): The hashing algorithm to use for checksum verification. Defaults to "md5". | |
| hashing_algorithm (str, optional): The hashing algorithm to use for checksum verification. Defaults to "md5". |
3 participants
This PR add the ability to have the Cisco IOS devices download the files instead of relying on copying the files to the device through scp. I am using the dataclass FilePullSpec to simplify the required arguments when downloading the files instead of copying them through scp.