From a6b0b834ea33c8bbb4d4c0bba2573cf41ab43ff4 Mon Sep 17 00:00:00 2001
From: Bittrance <bittrance@gmail.com>
Date: Fri, 6 Oct 2023 21:08:40 +0200
Subject: [PATCH] Ensure only successful tasks persist the sha.

Also, failing tasks dont abort execution.
---
 src/errors.rs           |  9 +++++++--
 src/lib.rs              | 13 +++++++++++++
 src/task/gixworkload.rs |  7 ++++++-
 src/testutils.rs        | 18 ++++++++++++++----
 tests/workload.rs       |  3 ++-
 5 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/src/errors.rs b/src/errors.rs
index 97df643..0f51ae9 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -38,6 +38,8 @@ pub enum GitOpsError {
     FetchError(Box<dyn std::error::Error + Send + Sync>),
     #[error("Failed to open repository: {0}")]
     OpenRepo(gix::open::Error),
+    #[error("Action failed: {1} in {0}")]
+    ActionFailed(String, String),
     #[error("Failed to send event: {0}")]
     NotifyError(String),
     #[error("Failed to launch action: {0}")]
@@ -57,7 +59,10 @@ pub enum GitOpsError {
 impl GitOpsError {
     #[allow(clippy::unused_self)]
     pub fn is_fatal(&self) -> bool {
-        // TODO Some errors should be recovered
-        true
+        #[allow(clippy::match_like_matches_macro)]
+        match self {
+            Self::ActionFailed(..) => false,
+            _ => true,
+        }
     }
 }
diff --git a/src/lib.rs b/src/lib.rs
index 0a42bd7..10f6842 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -76,6 +76,7 @@ mod lib {
     use gix::{hash::Kind, ObjectId};
 
     use crate::{
+        errors::GitOpsError,
         task::{scheduled::ScheduledTask, State},
         testutils::TestWorkload,
     };
@@ -106,4 +107,16 @@ mod lib {
         let progress = super::progress_one_task(&mut tasks[..], &mut persist).unwrap();
         assert!(progress == super::Progress::Idle);
     }
+
+    #[test]
+    fn dont_pesist_failing_task() {
+        let mut tasks = vec![ScheduledTask::new(TestWorkload::fail_with(|| {
+            GitOpsError::ActionFailed("ze-task".to_owned(), "ze-action".to_owned())
+        }))];
+        let mut persist = |_t: &ScheduledTask<TestWorkload>| Ok(());
+        super::progress_one_task(&mut tasks[..], &mut persist).unwrap();
+        tasks[0].await_finished();
+        super::progress_one_task(&mut tasks[..], &mut persist).unwrap();
+        assert_eq!(tasks[0].state().current_sha, ObjectId::null(Kind::Sha1));
+    }
 }
diff --git a/src/task/gixworkload.rs b/src/task/gixworkload.rs
index ff63944..068d098 100644
--- a/src/task/gixworkload.rs
+++ b/src/task/gixworkload.rs
@@ -96,6 +96,7 @@ impl Workload for GitWorkload {
                 new_sha,
             ))
             .map_err(|err| GitOpsError::NotifyError(format!("{}", err)))?;
+            // TODO The returns dodge cleanup
             match self.run_actions(&workdir, deadline, &sink) {
                 Ok(None) => {
                     sink.lock().unwrap()(WorkloadEvent::Success(self.config.name.clone(), new_sha))
@@ -104,10 +105,14 @@ impl Workload for GitWorkload {
                 Ok(Some(action_name)) => {
                     sink.lock().unwrap()(WorkloadEvent::Failure(
                         self.config.name.clone(),
-                        action_name,
+                        action_name.clone(),
                         new_sha,
                     ))
                     .map_err(|err| GitOpsError::NotifyError(format!("{}", err)))?;
+                    return Err(GitOpsError::ActionFailed(
+                        self.config.name.clone(),
+                        action_name,
+                    ));
                 }
                 Err(err) => {
                     sink.lock().unwrap()(WorkloadEvent::Error(
diff --git a/src/testutils.rs b/src/testutils.rs
index a62bc0f..1eb7c27 100644
--- a/src/testutils.rs
+++ b/src/testutils.rs
@@ -1,6 +1,6 @@
 use std::{
     path::PathBuf,
-    sync::{atomic::AtomicBool, Arc},
+    sync::Arc,
     thread::sleep,
     time::Duration,
 };
@@ -28,7 +28,16 @@ impl<W: Workload + Clone + Send + 'static> ScheduledTask<W> {
 
 #[derive(Clone, Default)]
 pub struct TestWorkload {
-    pub status: Arc<AtomicBool>,
+    errfunc: Option<Arc<Box<dyn Fn() -> GitOpsError + Send + Sync>>>,
+}
+
+impl TestWorkload {
+    pub fn fail_with(errfunc: impl Fn() -> GitOpsError + Send + Sync + 'static) -> Self {
+        Self {
+            errfunc: Some(Arc::new(Box::new(errfunc))),
+            ..Default::default()
+        }
+    }
 }
 
 impl Workload for TestWorkload {
@@ -41,9 +50,10 @@ impl Workload for TestWorkload {
     }
 
     fn perform(self, _workdir: PathBuf, _current_sha: ObjectId) -> Result<ObjectId, GitOpsError> {
-        self.status
-            .store(true, std::sync::atomic::Ordering::Relaxed);
         sleep(Duration::from_millis(10));
+        if self.errfunc.is_some() {
+            return Err(self.errfunc.unwrap()());
+        }
         Ok(ObjectId::empty_blob(gix::hash::Kind::Sha1))
     }
 }
diff --git a/tests/workload.rs b/tests/workload.rs
index 315b18e..207a1da 100644
--- a/tests/workload.rs
+++ b/tests/workload.rs
@@ -96,7 +96,8 @@ fn watch_failing_workload() {
         Ok(())
     });
     let prev_sha = ObjectId::empty_tree(Kind::Sha1);
-    workload.perform(workdir.into_path(), prev_sha).unwrap();
+    let res = workload.perform(workdir.into_path(), prev_sha);
+    assert!(matches!(res, Err(GitOpsError::ActionFailed(..))));
     let events = non_action_events(events);
     assert_eq!(events.len(), 2);
     assert!(matches!(events[0], WorkloadEvent::Changes(..)));