Merge pull request #205 from serokell/rvem/improve-temp-path-handling

[Chore] Handle 'temp_path' as an actual 'Path' instead of 'String'

5 files changed, 40 insertions, 31 deletions

diff --git a/src/bin/activate.rs b/src/bin/activate.rs
index 4c865f1..bf03538 100644
--- a/src/bin/activate.rs
+++ b/src/bin/activate.rs
@@ -15,9 +15,9 @@ use tokio::time::timeout;
 
 use std::time::Duration;
 
-use std::path::Path;
+use std::path::PathBuf;
 
-use notify::{RecommendedWatcher, RecursiveMode, Watcher};
+use notify::{RecommendedWatcher, RecursiveMode, Watcher, recommended_watcher};
 
 use thiserror::Error;
 
@@ -75,7 +75,7 @@ struct ActivateOpts {
 
     /// Path for any temporary files that may be needed during activation
     #[clap(long)]
-    temp_path: String,
+    temp_path: PathBuf,
 }
 
 /// Activate a profile
@@ -86,7 +86,7 @@ struct WaitOpts {
 
     /// Path for any temporary files that may be needed during activation
     #[clap(long)]
-    temp_path: String,
+    temp_path: PathBuf,
 }
 
 /// Activate a profile
@@ -232,7 +232,7 @@ async fn danger_zone(
 
 pub async fn activation_confirmation(
     profile_path: String,
-    temp_path: String,
+    temp_path: PathBuf,
     confirm_timeout: u16,
     closure: String,
 ) -> Result<(), ActivationConfirmationError> {
@@ -240,7 +240,7 @@ pub async fn activation_confirmation(
 
     debug!("Ensuring parent directory exists for canary file");
 
-    if let Some(parent) = Path::new(&lock_path).parent() {
+    if let Some(parent) = lock_path.parent() {
         fs::create_dir_all(parent)
             .await
             .map_err(ActivationConfirmationError::CreateConfirmDir)?;
@@ -257,7 +257,7 @@ pub async fn activation_confirmation(
     let (deleted, done) = mpsc::channel(1);
 
     let mut watcher: RecommendedWatcher =
-        Watcher::new_immediate(move |res: Result<notify::event::Event, notify::Error>| {
+        recommended_watcher(move |res: Result<notify::event::Event, notify::Error>| {
             let send_result = match res {
                 Ok(e) if e.kind == notify::EventKind::Remove(notify::event::RemoveKind::File) => {
                     debug!("Got worthy removal event, sending on channel");
@@ -298,7 +298,7 @@ pub enum WaitError {
     #[error("Error waiting for activation: {0}")]
     Waiting(#[from] DangerZoneError),
 }
-pub async fn wait(temp_path: String, closure: String) -> Result<(), WaitError> {
+pub async fn wait(temp_path: PathBuf, closure: String) -> Result<(), WaitError> {
     let lock_path = deploy::make_lock_path(&temp_path, &closure);
 
     let (created, done) = mpsc::channel(1);
@@ -307,11 +307,16 @@ pub async fn wait(temp_path: String, closure: String) -> Result<(), WaitError> {
         // TODO: fix wasteful clone
         let lock_path = lock_path.clone();
 
-        Watcher::new_immediate(move |res: Result<notify::event::Event, notify::Error>| {
+        recommended_watcher(move |res: Result<notify::event::Event, notify::Error>| {
             let send_result = match res {
                 Ok(e) if e.kind == notify::EventKind::Create(notify::event::CreateKind::File) => {
                     match &e.paths[..] {
-                        [x] if x == Path::new(&lock_path) => created.try_send(Ok(())),
+                        [x] => match lock_path.canonicalize() {
+                            // 'lock_path' may not exist yet when some other files are created in 'temp_path'
+                            // x is already supposed to be canonical path
+                            Ok(lock_path) if x == &lock_path => created.try_send(Ok(())),
+                            _ => Ok (())
+                        }
                         _ => Ok(()),
                     }
                 }
@@ -363,7 +368,7 @@ pub async fn activate(
     profile_path: String,
     closure: String,
     auto_rollback: bool,
-    temp_path: String,
+    temp_path: PathBuf,
     confirm_timeout: u16,
     magic_rollback: bool,
     dry_activate: bool,
diff --git a/src/cli.rs b/src/cli.rs
index f259563..9140d6d 100644
--- a/src/cli.rs
+++ b/src/cli.rs
@@ -14,6 +14,7 @@ use self::deploy::{DeployFlake, ParseFlakeError};
 use futures_util::stream::{StreamExt, TryStreamExt};
 use log::{debug, error, info, warn};
 use serde::Serialize;
+use std::path::PathBuf;
 use std::process::Stdio;
 use thiserror::Error;
 use tokio::process::Command;
@@ -86,7 +87,7 @@ pub struct Opts {
     confirm_timeout: Option<u16>,
     /// Where to store temporary files (only used by magic-rollback)
     #[clap(long)]
-    temp_path: Option<String>,
+    temp_path: Option<PathBuf>,
     /// Show what will be activated on the machines
     #[clap(long)]
     dry_activate: bool,
diff --git a/src/data.rs b/src/data.rs
index 90ea331..3b3e2c9 100644
--- a/src/data.rs
+++ b/src/data.rs
@@ -5,6 +5,7 @@
 use merge::Merge;
 use serde::Deserialize;
 use std::collections::HashMap;
+use std::path::PathBuf;
 
 #[derive(Deserialize, Debug, Clone, Merge)]
 pub struct GenericSettings {
@@ -25,7 +26,7 @@ pub struct GenericSettings {
     #[serde(rename(deserialize = "confirmTimeout"))]
     pub confirm_timeout: Option<u16>,
     #[serde(rename(deserialize = "tempPath"))]
-    pub temp_path: Option<String>,
+    pub temp_path: Option<PathBuf>,
     #[serde(rename(deserialize = "magicRollback"))]
     pub magic_rollback: Option<bool>,
     #[serde(rename(deserialize = "sudo"))]
diff --git a/src/deploy.rs b/src/deploy.rs
index cc5d862..574e9b2 100644
--- a/src/deploy.rs
+++ b/src/deploy.rs
@@ -5,7 +5,7 @@
 // SPDX-License-Identifier: MPL-2.0
 
 use log::{debug, info};
-use std::borrow::Cow;
+use std::path::Path;
 use thiserror::Error;
 use tokio::process::Command;
 
@@ -16,7 +16,7 @@ struct ActivateCommandData<'a> {
     profile_path: &'a str,
     closure: &'a str,
     auto_rollback: bool,
-    temp_path: &'a str,
+    temp_path: &'a Path,
     confirm_timeout: u16,
     magic_rollback: bool,
     debug_logs: bool,
@@ -38,7 +38,7 @@ fn build_activate_command(data: &ActivateCommandData) -> String {
 
     self_activate_command = format!(
         "{} activate '{}' '{}' --temp-path '{}'",
-        self_activate_command, data.closure, data.profile_path, data.temp_path
+        self_activate_command, data.closure, data.profile_path, data.temp_path.display()
     );
 
     self_activate_command = format!(
@@ -77,7 +77,7 @@ fn test_activation_command_builder() {
     let auto_rollback = true;
     let dry_activate = false;
     let boot = false;
-    let temp_path = "/tmp";
+    let temp_path = Path::new("/tmp");
     let confirm_timeout = 30;
     let magic_rollback = true;
     let debug_logs = true;
@@ -105,7 +105,7 @@ fn test_activation_command_builder() {
 struct WaitCommandData<'a> {
     sudo: &'a Option<String>,
     closure: &'a str,
-    temp_path: &'a str,
+    temp_path: &'a Path,
     debug_logs: bool,
     log_dir: Option<&'a str>,
 }
@@ -123,7 +123,7 @@ fn build_wait_command(data: &WaitCommandData) -> String {
 
     self_activate_command = format!(
         "{} wait '{}' --temp-path '{}'",
-        self_activate_command, data.closure, data.temp_path,
+        self_activate_command, data.closure, data.temp_path.display(),
     );
 
     if let Some(sudo_cmd) = &data.sudo {
@@ -137,7 +137,7 @@ fn build_wait_command(data: &WaitCommandData) -> String {
 fn test_wait_command_builder() {
     let sudo = Some("sudo -u test".to_string());
     let closure = "/nix/store/blah/etc";
-    let temp_path = "/tmp";
+    let temp_path = Path::new("/tmp");
     let debug_logs = true;
     let log_dir = Some("/tmp/something.txt");
 
@@ -216,7 +216,7 @@ pub enum ConfirmProfileError {
 pub async fn confirm_profile(
     deploy_data: &super::DeployData<'_>,
     deploy_defs: &super::DeployDefs,
-    temp_path: Cow<'_, str>,
+    temp_path: &Path,
     ssh_addr: &str,
 ) -> Result<(), ConfirmProfileError> {
     let mut ssh_confirm_command = Command::new("ssh");
@@ -226,9 +226,9 @@ pub async fn confirm_profile(
         ssh_confirm_command.arg(ssh_opt);
     }
 
-    let lock_path = super::make_lock_path(&temp_path, &deploy_data.profile.profile_settings.path);
+    let lock_path = super::make_lock_path(temp_path, &deploy_data.profile.profile_settings.path);
 
-    let mut confirm_command = format!("rm {}", lock_path);
+    let mut confirm_command = format!("rm {}", lock_path.display());
     if let Some(sudo_cmd) = &deploy_defs.sudo {
         confirm_command = format!("{} {}", sudo_cmd, confirm_command);
     }
@@ -286,9 +286,9 @@ pub async fn deploy_profile(
         );
     }
 
-    let temp_path: Cow<str> = match &deploy_data.merged_settings.temp_path {
-        Some(x) => x.into(),
-        None => "/tmp".into(),
+    let temp_path: &Path = match &deploy_data.merged_settings.temp_path {
+        Some(x) => x,
+        None => Path::new("/tmp"),
     };
 
     let confirm_timeout = deploy_data.merged_settings.confirm_timeout.unwrap_or(30);
@@ -302,7 +302,7 @@ pub async fn deploy_profile(
         profile_path: &deploy_defs.profile_path,
         closure: &deploy_data.profile.profile_settings.path,
         auto_rollback,
-        temp_path: &temp_path,
+        temp_path: temp_path,
         confirm_timeout,
         magic_rollback,
         debug_logs: deploy_data.debug_logs,
@@ -350,7 +350,7 @@ pub async fn deploy_profile(
         let self_wait_command = build_wait_command(&WaitCommandData {
             sudo: &deploy_defs.sudo,
             closure: &deploy_data.profile.profile_settings.path,
-            temp_path: &temp_path,
+            temp_path: temp_path,
             debug_logs: deploy_data.debug_logs,
             log_dir: deploy_data.log_dir,
         });
diff --git a/src/lib.rs b/src/lib.rs
index 738fa81..bf1a6dc 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -12,10 +12,12 @@ use thiserror::Error;
 
 use flexi_logger::*;
 
-pub fn make_lock_path(temp_path: &str, closure: &str) -> String {
+use std::path::{Path, PathBuf};
+
+pub fn make_lock_path(temp_path: &Path, closure: &str) -> PathBuf {
     let lock_hash =
         &closure["/nix/store/".len()..closure.find('-').unwrap_or_else(|| closure.len())];
-    format!("{}/deploy-rs-canary-{}", temp_path, lock_hash)
+    temp_path.join(format!("deploy-rs-canary-{}", lock_hash))
 }
 
 const fn make_emoji(level: log::Level) -> &'static str {
@@ -159,7 +161,7 @@ pub struct CmdOverrides {
     pub auto_rollback: Option<bool>,
     pub hostname: Option<String>,
     pub magic_rollback: Option<bool>,
-    pub temp_path: Option<String>,
+    pub temp_path: Option<PathBuf>,
     pub confirm_timeout: Option<u16>,
     pub sudo: Option<String>,
     pub dry_activate: bool,