From d0a14d7763bcf36437423cc74bb83d5be3d13fc1 Mon Sep 17 00:00:00 2001 From: Leonard Steppy Date: Tue, 17 Dec 2024 06:19:09 +0100 Subject: [PATCH 1/8] Add command module --- src/command.rs | 31 +++++++++++++++++++++++++++++++ src/main.rs | 1 + 2 files changed, 32 insertions(+) create mode 100644 src/command.rs diff --git a/src/command.rs b/src/command.rs new file mode 100644 index 0000000..acb3c27 --- /dev/null +++ b/src/command.rs @@ -0,0 +1,31 @@ +use crate::logger::Logger; +use std::fmt::{Display, Formatter}; +use std::io; +use std::process::{Command, ExitStatus}; + +pub trait LogRunnable { + fn run(&mut self, logger: &Logger) -> Result<(), ExecutionError>; +} + +impl LogRunnable for Command { + //TODO use specific execution error as error type + fn run(&mut self, logger: &Logger) -> Result<(), ExecutionError> { + todo!("depending on log level, pipe output and only print with logger on error") + } +} + +//TODO show command in specific execution error +#[derive(Debug)] +pub enum ExecutionError { + StartError(io::Error), + BadExitStatus(ExitStatus), +} + +impl Display for ExecutionError { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + match self { + ExecutionError::StartError(e) => write!(f, "failed to start command: {}", e), + ExecutionError::BadExitStatus(status) => write!(f, "command failed with exit status: {}", status), + } + } +} \ No newline at end of file diff --git a/src/main.rs b/src/main.rs index b9ec631..e775d74 100644 --- a/src/main.rs +++ b/src/main.rs @@ -3,6 +3,7 @@ mod file; mod logger; mod os_string_builder; mod server; +mod command; use crate::action::{Action, FileAction, ServerActions}; use crate::file::{FileMatcher, FileNameInfo}; From f3e965394ed305698129ec5e419404384c2ea8de Mon Sep 17 00:00:00 2001 From: Leonard Steppy Date: Tue, 17 Dec 2024 10:31:02 +0100 Subject: [PATCH 2/8] Implement command module --- src/command.rs | 78 ++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 70 insertions(+), 8 deletions(-) diff --git a/src/command.rs b/src/command.rs index acb3c27..de280eb 100644 --- a/src/command.rs +++ b/src/command.rs @@ -1,31 +1,93 @@ -use crate::logger::Logger; +use crate::log; +use crate::logger::{LogLevel, Logger}; +use std::error::Error; use std::fmt::{Display, Formatter}; use std::io; -use std::process::{Command, ExitStatus}; +use std::process::{Command, ExitStatus, Stdio}; pub trait LogRunnable { fn run(&mut self, logger: &Logger) -> Result<(), ExecutionError>; } impl LogRunnable for Command { - //TODO use specific execution error as error type - fn run(&mut self, logger: &Logger) -> Result<(), ExecutionError> { - todo!("depending on log level, pipe output and only print with logger on error") + fn run(&mut self, logger: &Logger) -> Result<(), SpecificExecutionError> { + //TODO I'm not happy yet with the implementation of this method + match logger.level { + LogLevel::Debug | LogLevel::Info => self + .status() + .map_err(ExecutionError::StartError) + .and_then(|status| { + if status.success() { + Ok(()) + } else { + Err(ExecutionError::BadExitStatus(status)) + } + }), + LogLevel::Error => self + .stdout(Stdio::piped()) + .output() + .map_err(ExecutionError::StartError) + .and_then(|output| { + if output.status.success() { + Ok(()) + } else { + log!(logger, error, "{}", String::from_utf8_lossy(&output.stderr)); + Err(ExecutionError::BadExitStatus(output.status)) + } + }), + } + .map_err(|error| SpecificExecutionError { + command: self, + error, + }) } } -//TODO show command in specific execution error +#[derive(Debug)] +pub struct SpecificExecutionError<'a> { + pub command: &'a Command, + pub error: ExecutionError, +} + +impl Display for SpecificExecutionError<'_> { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!( + f, + "Failed to execute command {}: {}", + self.command, self.error + ) + } +} + +impl Error for SpecificExecutionError<'_> {} + #[derive(Debug)] pub enum ExecutionError { StartError(io::Error), BadExitStatus(ExitStatus), } +impl From for ExecutionError { + fn from(value: io::Error) -> Self { + Self::StartError(value) + } +} + +impl From for ExecutionError { + fn from(value: ExitStatus) -> Self { + Self::BadExitStatus(value) + } +} + impl Display for ExecutionError { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { match self { ExecutionError::StartError(e) => write!(f, "failed to start command: {}", e), - ExecutionError::BadExitStatus(status) => write!(f, "command failed with exit status: {}", status), + ExecutionError::BadExitStatus(status) => { + write!(f, "command failed with exit status: {}", status) + } } } -} \ No newline at end of file +} + +impl Error for ExecutionError {} From 9befc1fcf275eb547f11f561d0d10807f7ff7ad4 Mon Sep 17 00:00:00 2001 From: Leonard Steppy Date: Tue, 17 Dec 2024 15:29:35 +0100 Subject: [PATCH 3/8] Fix and refactor command module --- src/command.rs | 52 ++++++++++++++++++++++---------------------------- 1 file changed, 23 insertions(+), 29 deletions(-) diff --git a/src/command.rs b/src/command.rs index de280eb..8b9e2dc 100644 --- a/src/command.rs +++ b/src/command.rs @@ -6,43 +6,37 @@ use std::io; use std::process::{Command, ExitStatus, Stdio}; pub trait LogRunnable { - fn run(&mut self, logger: &Logger) -> Result<(), ExecutionError>; + fn run(&mut self, logger: &Logger) -> Result<(), SpecificExecutionError>; } impl LogRunnable for Command { fn run(&mut self, logger: &Logger) -> Result<(), SpecificExecutionError> { - //TODO I'm not happy yet with the implementation of this method - match logger.level { - LogLevel::Debug | LogLevel::Info => self - .status() - .map_err(ExecutionError::StartError) - .and_then(|status| { - if status.success() { - Ok(()) - } else { - Err(ExecutionError::BadExitStatus(status)) - } - }), - LogLevel::Error => self - .stdout(Stdio::piped()) - .output() - .map_err(ExecutionError::StartError) - .and_then(|output| { - if output.status.success() { - Ok(()) - } else { - log!(logger, error, "{}", String::from_utf8_lossy(&output.stderr)); - Err(ExecutionError::BadExitStatus(output.status)) - } - }), - } - .map_err(|error| SpecificExecutionError { + run(self, logger).map_err(|error| SpecificExecutionError { command: self, error, }) } } +fn run(command: &mut Command, logger: &Logger) -> Result<(), ExecutionError> { + match logger.level { + LogLevel::Debug | LogLevel::Info => { + let status = command.status()?; + if !status.success() { + Err(status)?; + } + } + LogLevel::Error => { + let output = command.stdout(Stdio::piped()).output()?; + if !output.status.success() { + log!(logger, error, "{}", String::from_utf8_lossy(&output.stderr)); + Err(output.status)?; + } + } + } + Ok(()) +} + #[derive(Debug)] pub struct SpecificExecutionError<'a> { pub command: &'a Command, @@ -53,7 +47,7 @@ impl Display for SpecificExecutionError<'_> { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!( f, - "Failed to execute command {}: {}", + "Failed to execute command {:?}: {}", self.command, self.error ) } @@ -90,4 +84,4 @@ impl Display for ExecutionError { } } -impl Error for ExecutionError {} +impl Error for ExecutionError {} \ No newline at end of file From b6002b635d8491ac6ffbcea46562bd9a6c2f0b92 Mon Sep 17 00:00:00 2001 From: Leonard Steppy Date: Tue, 17 Dec 2024 15:40:53 +0100 Subject: [PATCH 4/8] Add unit test to command module --- src/command.rs | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/src/command.rs b/src/command.rs index 8b9e2dc..3e3f5f1 100644 --- a/src/command.rs +++ b/src/command.rs @@ -84,4 +84,29 @@ impl Display for ExecutionError { } } -impl Error for ExecutionError {} \ No newline at end of file +impl Error for ExecutionError {} + +#[cfg(test)] +mod test { + use crate::command::{ExecutionError, LogRunnable, SpecificExecutionError}; + use crate::logger::Logger; + use std::process::Command; + + #[test] + fn test_unknown_command() { + let mut command = Command::new("a_command_which_def_doesnt_exist"); + let Err( + e @ SpecificExecutionError { + error: ExecutionError::StartError(_), + .. + }, + ) = command.args(["foo", "bar"]).run(&Logger::default()) + else { + panic!("command shouldn't exist"); + }; + println!("{e}"); + } + + #[test] + fn test_error() {} +} From 0c56e837d431666539593f83562a99d14f50d2b0 Mon Sep 17 00:00:00 2001 From: Steppy Date: Wed, 18 Dec 2024 11:35:38 +0100 Subject: [PATCH 5/8] Improve command error messages and unit test coverage --- README.md | 4 +++ src/command.rs | 42 ++++++++++++++++++++++++++------ src/logger.rs | 2 +- test-ressources/python/exit_1.py | 1 + 4 files changed, 40 insertions(+), 9 deletions(-) create mode 100644 test-ressources/python/exit_1.py diff --git a/README.md b/README.md index 66e0b1b..0dfed25 100644 --- a/README.md +++ b/README.md @@ -78,3 +78,7 @@ Once you have that installed, just run cargo build --release ``` and you will find an executable in `target/release`. + +### Unit tests + +In order for the unit tests to pass, you will need `python3` diff --git a/src/command.rs b/src/command.rs index 3e3f5f1..be7431f 100644 --- a/src/command.rs +++ b/src/command.rs @@ -3,6 +3,7 @@ use crate::logger::{LogLevel, Logger}; use std::error::Error; use std::fmt::{Display, Formatter}; use std::io; +use std::iter::once; use std::process::{Command, ExitStatus, Stdio}; pub trait LogRunnable { @@ -47,12 +48,20 @@ impl Display for SpecificExecutionError<'_> { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { write!( f, - "Failed to execute command {:?}: {}", - self.command, self.error + "Failed to execute command '{}': {}", + command_to_string(self.command), + self.error ) } } +fn command_to_string(command: &Command) -> String { + once(command.get_program().to_string_lossy()) + .chain(command.get_args().map(|arg| arg.to_string_lossy())) + .collect::>() + .join(" ") +} + impl Error for SpecificExecutionError<'_> {} #[derive(Debug)] @@ -76,9 +85,9 @@ impl From for ExecutionError { impl Display for ExecutionError { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { match self { - ExecutionError::StartError(e) => write!(f, "failed to start command: {}", e), + ExecutionError::StartError(e) => write!(f, "Failed to start command: {}", e), ExecutionError::BadExitStatus(status) => { - write!(f, "command failed with exit status: {}", status) + write!(f, "Command failed with {}", status) } } } @@ -90,23 +99,40 @@ impl Error for ExecutionError {} mod test { use crate::command::{ExecutionError, LogRunnable, SpecificExecutionError}; use crate::logger::Logger; + use std::path::PathBuf; use std::process::Command; #[test] fn test_unknown_command() { - let mut command = Command::new("a_command_which_def_doesnt_exist"); + let mut command = Command::new("python7"); let Err( e @ SpecificExecutionError { error: ExecutionError::StartError(_), .. }, - ) = command.args(["foo", "bar"]).run(&Logger::default()) + ) = command + .args([PathBuf::from("test-ressources/python/exit_1.py")]) + .run(&Logger::default()) else { panic!("command shouldn't exist"); }; - println!("{e}"); + assert_eq!(e.to_string(), "Failed to execute command 'python7': Failed to start command: No such file or directory (os error 2)"); } #[test] - fn test_error() {} + fn test_error() { + let mut command = Command::new("python3"); + let Err( + e @ SpecificExecutionError { + error: ExecutionError::BadExitStatus(_), + .. + }, + ) = command + .arg("test-ressources/python/exit_1.py") + .run(&Logger::default()) + else { + panic!("command should return exit-code 1") + }; + assert_eq!(e.to_string(), "Failed to execute command 'python3 test-ressources/python/exit_1.py': Command failed with exit status: 1"); + } } diff --git a/src/logger.rs b/src/logger.rs index 89226b6..57ddae5 100644 --- a/src/logger.rs +++ b/src/logger.rs @@ -39,7 +39,7 @@ macro_rules! log { $logger.$level(format!($($args)*)); }; ($logger:expr, $($args:tt)*) => { - log!($logger, info, $($args)*); + log!($logger, info, $($args)*); //TODO better use default level with log function instead of assuming info as default } } diff --git a/test-ressources/python/exit_1.py b/test-ressources/python/exit_1.py new file mode 100644 index 0000000..b11a11b --- /dev/null +++ b/test-ressources/python/exit_1.py @@ -0,0 +1 @@ +exit(1) \ No newline at end of file From fefdffe1e90278f61a6a0ef4722b3ecb87c4f5cc Mon Sep 17 00:00:00 2001 From: Leonard Steppy Date: Wed, 8 Jan 2025 10:24:15 +0100 Subject: [PATCH 6/8] Use run function for commands where possible --- src/command.rs | 1 + src/main.rs | 51 ++++++++++++++++++++------------------------------ 2 files changed, 21 insertions(+), 31 deletions(-) diff --git a/src/command.rs b/src/command.rs index be7431f..529ce24 100644 --- a/src/command.rs +++ b/src/command.rs @@ -7,6 +7,7 @@ use std::iter::once; use std::process::{Command, ExitStatus, Stdio}; pub trait LogRunnable { + //TODO add function to query output on success, could further improve code readability fn run(&mut self, logger: &Logger) -> Result<(), SpecificExecutionError>; } diff --git a/src/main.rs b/src/main.rs index e775d74..ef7d7b9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,11 +1,12 @@ mod action; +mod command; mod file; mod logger; mod os_string_builder; mod server; -mod command; use crate::action::{Action, FileAction, ServerActions}; +use crate::command::LogRunnable; use crate::file::{FileMatcher, FileNameInfo}; use crate::logger::{LogLevel, Logger}; use crate::os_string_builder::ReplaceWithOsStr; @@ -160,7 +161,7 @@ fn main() -> Result<(), String> { file_name, } => { require_non_empty_servers(&servers)?; - start_ssh_agent()?; + start_ssh_agent(&logger)?; let file_name_info = FileNameInfo::try_from(file.clone()).map_err(|e| format!("bad file: {e}"))?; @@ -275,19 +276,15 @@ fn main() -> Result<(), String> { ShellCmd::new("scp") .arg(file.clone()) .arg(osf!(&server.ssh_name) + ":" + &server_actions.working_directory) - .spawn() - .map_err(|e| format!("failed to upload file: {e}"))? - .wait() - .map_err(|e| format!("failed to wait for upload: {e}"))?; + .run(&logger) + .map_err(|e| format!("upload failure: {e}"))?; } Action::Delete => { ShellCmd::new("ssh") .arg(&server.ssh_name) .arg(osf!("cd ") + &server_actions.working_directory + "; rm " + &file_action.file) - .spawn() - .map_err(|e| format!("failed to send delete command: {e}"))? - .wait() - .map_err(|e| format!("failed to wait for delete command: {e}"))?; + .run(&logger) + .map_err(|e| format!("failed to delete old version: {e}"))?; } Action::Rename { new_name } => { ShellCmd::new("ssh") @@ -300,10 +297,8 @@ fn main() -> Result<(), String> { + " " + new_name, ) - .spawn() - .map_err(|e| format!("failed to send rename command: {e}"))? - .wait() - .map_err(|e| format!("failed to wait for rename command: {e}"))?; + .run(&logger) + .map_err(|e| format!("failed to rename: {e}"))?; } } } @@ -312,17 +307,15 @@ fn main() -> Result<(), String> { log!(logger, "Done!"); } Command::Command { command } => { - start_ssh_agent()?; + start_ssh_agent(&logger)?; require_non_empty_servers(&servers)?; for server in servers { log!(logger, "Running command on '{}'...", server.ssh_name); ShellCmd::new("ssh") .arg(server.ssh_name) .arg(osf!("cd ") + server.server_directory_path + "; " + &command) - .spawn() - .map_err(|_| "failed to start ssh command".to_string())? - .wait() - .map_err(|e| format!("failed to wait for ssh command completion: {e}"))?; + .run(&logger) + .map_err(|e| format!("{e}"))?; } log!(logger, "Done!"); } @@ -358,15 +351,15 @@ fn main() -> Result<(), String> { } require_non_empty_servers(&servers)?; - start_ssh_agent()?; + start_ssh_agent(&logger)?; for server in servers { log!(logger, "Downloading file from {}...", server.ssh_name); ShellCmd::new("scp") .arg(osf!(&server.ssh_name) + ":" + server.server_directory_path.join(&file)) .arg(&working_directory) - .status() - .map_err(|e| format!("failed to download file: {e}"))?; + .run(&logger) + .map_err(|e| format!("download failure: {e}"))?; //open file in editor let mut shell_args = shell_words::split(&editor) @@ -378,15 +371,15 @@ fn main() -> Result<(), String> { let command = shell_args.remove(0); ShellCmd::new(command) .args(shell_args) - .status() + .run(&logger) .map_err(|e| format!("failed to open file in editor: {e}"))?; //upload file again ShellCmd::new("scp") .arg(working_directory.join(file_name)) .arg(osf!(&server.ssh_name) + ":" + server.server_directory_path.join(&file)) - .status() - .map_err(|e| format!("failed to upload file again: {e}"))?; + .run(&logger) + .map_err(|e| format!("failed to re-upload file: {e}"))?; } log!(logger, "Done!"); @@ -404,7 +397,7 @@ fn require_non_empty_servers(servers: &[Server]) -> Result<(), String> { } } -fn start_ssh_agent() -> Result<(), String> { +fn start_ssh_agent(logger: &Logger) -> Result<(), String> { //start the ssh agent let agent_output = ShellCmd::new("ssh-agent") .arg("-s") @@ -424,11 +417,7 @@ fn start_ssh_agent() -> Result<(), String> { } //add the ssh key - ShellCmd::new("ssh-add") - .spawn() - .map_err(|e| format!("failed to add ssh key: {}", e))? - .wait() - .expect("failed to wait on ssh-add"); + ShellCmd::new("ssh-add").run(logger).map_err(|e| format!("failed to add ssh-key: {e}"))?; Ok(()) } From a932a9eb67e2c3ec32cfc196fa68a562c2605dcc Mon Sep 17 00:00:00 2001 From: Leonard Steppy Date: Wed, 8 Jan 2025 13:38:51 +0100 Subject: [PATCH 7/8] Add collect_output function to shell commands --- src/command.rs | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/src/command.rs b/src/command.rs index 529ce24..4b9073d 100644 --- a/src/command.rs +++ b/src/command.rs @@ -4,11 +4,11 @@ use std::error::Error; use std::fmt::{Display, Formatter}; use std::io; use std::iter::once; -use std::process::{Command, ExitStatus, Stdio}; +use std::process::{Command, ExitStatus, Output}; pub trait LogRunnable { - //TODO add function to query output on success, could further improve code readability fn run(&mut self, logger: &Logger) -> Result<(), SpecificExecutionError>; + fn collect_output(&mut self) -> Result; } impl LogRunnable for Command { @@ -18,6 +18,13 @@ impl LogRunnable for Command { error, }) } + + fn collect_output(&mut self) -> Result { + collect_output(self, None).map_err(|error| SpecificExecutionError { + command: self, + error, + }) + } } fn run(command: &mut Command, logger: &Logger) -> Result<(), ExecutionError> { @@ -29,16 +36,26 @@ fn run(command: &mut Command, logger: &Logger) -> Result<(), ExecutionError> { } } LogLevel::Error => { - let output = command.stdout(Stdio::piped()).output()?; - if !output.status.success() { - log!(logger, error, "{}", String::from_utf8_lossy(&output.stderr)); - Err(output.status)?; - } + collect_output(command, Some(logger))?; } } Ok(()) } +fn collect_output( + command: &mut Command, + logger: Option<&Logger>, +) -> Result { + let output = command.output()?; //pipes stdout and stderr automatically + if !output.status.success() { + if let Some(logger) = logger { + log!(logger, error, "{}", String::from_utf8_lossy(&output.stderr)); + } + Err(output.status)?; + } + Ok(output) +} + #[derive(Debug)] pub struct SpecificExecutionError<'a> { pub command: &'a Command, From 51c82865652c110bbd99e9b43cbb866d43f23ca0 Mon Sep 17 00:00:00 2001 From: Leonard Steppy Date: Wed, 8 Jan 2025 13:43:05 +0100 Subject: [PATCH 8/8] Use collect_output where possible --- src/main.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/main.rs b/src/main.rs index ef7d7b9..b9918a4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -18,7 +18,6 @@ use std::hash::Hash; use std::io::Write; use std::iter::once; use std::path::PathBuf; -use std::process::Stdio; use std::str::FromStr; use std::{env, fs}; @@ -177,9 +176,8 @@ fn main() -> Result<(), String> { let output = ShellCmd::new("ssh") .arg(&server.ssh_name) .arg(osf!("ls ") + &working_directory) - .stdout(Stdio::piped()) - .output() - .map_err(|e| format!("failed to query files via ssh: {e}"))?; + .collect_output() + .map_err(|e| format!("failed to query files: {e}"))?; let output = String::from_utf8_lossy(&output.stdout); let mut file_matcher = @@ -401,8 +399,7 @@ fn start_ssh_agent(logger: &Logger) -> Result<(), String> { //start the ssh agent let agent_output = ShellCmd::new("ssh-agent") .arg("-s") - .stdout(Stdio::piped()) - .output() + .collect_output() .map_err(|e| format!("failed to start ssh agent: {e}"))?; let agent_stdout = String::from_utf8_lossy(&agent_output.stdout); if !agent_output.status.success() { @@ -417,7 +414,9 @@ fn start_ssh_agent(logger: &Logger) -> Result<(), String> { } //add the ssh key - ShellCmd::new("ssh-add").run(logger).map_err(|e| format!("failed to add ssh-key: {e}"))?; + ShellCmd::new("ssh-add") + .run(logger) + .map_err(|e| format!("failed to add ssh-key: {e}"))?; Ok(()) }