issue:4290882 Create generic functions in ufm_sdk_tools for general use and uses in this pytest #9
Conversation
Signed-off-by: Elad Gershon <[email protected]>
| print(f"failed to save {filename} at {location_on_remote}, got: {exception}") | ||
| return | ||
| if not is_localhost(hostname): | ||
| copy_file_to_remote(filename,location_on_remote,hostname) |
There was a problem hiding this comment.
please add a space after a comma
| if not is_localhost(hostname): | ||
| copy_file_to_remote(filename,location_on_remote,hostname) | ||
| else: | ||
| copy_file_to_host(filename,location_on_remote) |
There was a problem hiding this comment.
please add a space after a comma
| src (str): srouce location | ||
| destination (str): destination location for the file. | ||
| """ | ||
| os.makedirs(os.path.dirname(destination), exist_ok=True) |
There was a problem hiding this comment.
please put this in a try catch
| os.makedirs(os.path.dirname(destination), exist_ok=True) | ||
| shutil.copy(src, destination) | ||
|
|
||
| def copy_file_to_remote(local_file:str, remote_path:str, server:str, status=False, username:str="root", password:str="") -> int: |
There was a problem hiding this comment.
please remove the root default.
| if not is_folder_exists(remote_path, server, username, password): | ||
| run_command_status(f"mkdir -p {remote_path}", server) | ||
| if ":" in server: | ||
| cmd = f"scp -6 {local_file} root@[{server}]:{remote_path}" |
There was a problem hiding this comment.
you can't use root here. why are you using ip6?
Please explain.
| return (proc.returncode, out, err) | ||
|
|
||
|
|
||
| def run_ssh_command(command, device=None, verbose=True, username:str="root"): |
There was a problem hiding this comment.
Remove root from the default variables.
| bool: if the device is localhost or 127.0.0.1 | ||
| """ | ||
| if device is None or len(device) == 0: | ||
| return True |
There was a problem hiding this comment.
Why is an empty device a localhost?
|
|
||
| logging.info("Status Code is As Expected") | ||
|
|
||
| def kill_proc_by_name(proc_name:str, exclude_proc:str=None, kill_flag:str="-9", device:str=None, raise_exception=True)->int: |
There was a problem hiding this comment.
This design for the function is incorrect.
You can't return the pids that need to be killed. It doesn't align with the purpose of the function.
| Exception: if there was an issue killing the process. | ||
|
|
||
| Returns: | ||
| int: if kill_flag is -9: 0 is kill was successful, 1 else. otherwise the pids that needs to be killed |
There was a problem hiding this comment.
This design is error-pruned and doesn't meet good practices of automation.
Please refactor this method.
| IFCONFIG_IB0 = """/sbin/ifconfig ib0 |grep -B1 "inet" |awk '{ if ( $1 == "inet" ) { print $2 } else if ( $2 == "Link" ) { printf "%s:" ,$1 } }' |awk -F: '{ print $1 ": " $3 }'""" | ||
| IFCONFIG = "ifconfig" | ||
| SSH_CMD = "ssh -o 'StrictHostKeyChecking=no' {user_name}@{device} '{command}'" | ||
| SSH_CMD_IPV6 = "ssh -6 'StrictHostKeyChecking=no' {user_name}@{device} '{command}'" |
There was a problem hiding this comment.
why do you need to support ip v6?
| """ | ||
|
|
||
| try: | ||
| file = open(filename, "w", encoding="utf-8") |
There was a problem hiding this comment.
I would use with open so you know he file is closed, other wise, you need to add a finally block to check if we need to close the file
| file.write(content) | ||
| file.close() | ||
| except IOError as exception: | ||
| print(f"failed to save {filename} at {location_on_remote}, got: {exception}") |
| except IOError as exception: | ||
| print(f"failed to save {filename} at {location_on_remote}, got: {exception}") | ||
| return | ||
| if not is_localhost(hostname): |
There was a problem hiding this comment.
Can you make it a positive if? I think it will make the code more readable :
if is_locahost(hostname):
copy_file_to_host(filename,location_on_remote)
else ...
| destination (str): destination location for the file. | ||
| """ | ||
| os.makedirs(os.path.dirname(destination), exist_ok=True) | ||
| shutil.copy(src, destination) |
There was a problem hiding this comment.
I think that each step in the function needs validation to succeed and proper logging.
The caller does not care if it failed? or do you mean the caller should catch the exception? If that, I would write it in the doc string
| int: _description_ | ||
| """ | ||
| if not is_folder_exists(remote_path, server, username, password): | ||
| run_command_status(f"mkdir -p {remote_path}", server) |
| if not multi_line_prefix_cmd: | ||
| command = f"docker exec {docker_option} {container_name} {command}" | ||
| else: | ||
| command = f'docker exec {docker_option} {container_name} bash -c "{command}"' |
There was a problem hiding this comment.
To keep it consistent with other f"", I would change to
command = f"docker exec {docker_option} {container_name} bash -c \"{command}\""
| command = f"cd /tmp; sudo -u {under_user} {command}" | ||
|
|
||
| if not is_localhost(device): | ||
| returncode, stdout, stderr, conn = (None,) * 4 |
There was a problem hiding this comment.
I think returncode = stdout = stderr = conn = None is a bit more clear
| conn = connect_to_remote(device, user, password) | ||
| connection_error = False | ||
| except Exception as e: | ||
| print(e) |
| channel = '' | ||
| timeout = 130 | ||
| for idx in range(0, 10): | ||
| endtime = time.time() + timeout |
There was a problem hiding this comment.
time.perf() is be usually better to check time delta
| stderr = err.read() | ||
| break | ||
| else: | ||
| logging.info("Channel Closed") |
There was a problem hiding this comment.
Which channel? will the user get any value from this log? maybe add information
What
Add common functions for use in ufm_sdk_3.0
Why
Task
Those common functions are useful for any plugin pytest.
How
Added common functions that can copy, execute, check existing folders, and kill processes on local and remote machines. the shell commands allow addition to run other commands if needed.
Testing
This is part of the testing