feat: annotations in skctl validate include possible patches instea…

…d of just affected indices

sk-cli/src/validation/annotated_trace.rs

@@ -1,4 +1,4 @@
-use std::collections::BTreeMap;
+use std::collections::BTreeMap; // BTreeMap sorts by key, HashMap doesn't
 use std::slice;
 
 use json_patch_ext::prelude::*;
@@ -18,37 +18,50 @@ use super::validator::{
     ValidatorCode,
 };
 
-#[derive(Clone, Default)]
+#[derive(Clone, Debug)]
+pub enum PatchLocations {
+    Everywhere,
+    #[allow(dead_code)]
+    ObjectReference(TypeMeta, String),
+}
+
+#[derive(Clone, Debug)]
+pub struct AnnotatedTracePatch {
+    pub locations: PatchLocations,
+    pub ops: Vec<PatchOperation>,
+}
+
+#[derive(Clone, Debug)]
+pub struct Annotation {
+    pub code: ValidatorCode,
+    // The annotation applies to a particular object within an event; the patches
+    // vector is a list of _possible_ (and probably mutually-exclusive) patches
+    // that we can apply to the object that will fix the validation issue.  The first
+    // patch in this list is the "recommended" fix, in that this is the one that will
+    // be applied by `skctl validate check --fix`
+    pub patches: Vec<AnnotatedTracePatch>,
+}
+
+#[derive(Clone, Debug, Default)]
 pub struct AnnotatedTraceEvent {
     pub data: TraceEvent,
-    pub annotations: Vec<Vec<ValidatorCode>>,
+    // The annotations map is from "object index" to a list of problems/annotations
+    // that apply at that specific index (remember that the "object index" is interpreted
+    // as an applied object if it is less than data.applied_objs.len(), and as a deleted
+    // object otherwise.
+    pub annotations: BTreeMap<usize, Vec<Annotation>>,
 }
 
 impl AnnotatedTraceEvent {
     pub fn new(data: TraceEvent) -> AnnotatedTraceEvent {
-        let annotations = vec![vec![]; data.len()];
-
-        AnnotatedTraceEvent { data, annotations }
+        AnnotatedTraceEvent { data, ..Default::default() }
     }
 
     pub fn clear_annotations(&mut self) {
-        self.annotations = vec![vec![]; self.data.len()];
+        self.annotations.clear();
     }
 }
 
-pub enum PatchLocations {
-    #[allow(dead_code)]
-    Everywhere,
-    AffectedObjects(ValidatorCode),
-    #[allow(dead_code)]
-    ObjectReference(TypeMeta, String),
-}
-
-pub struct AnnotatedTracePatch {
-    pub locations: PatchLocations,
-    pub op: PatchOperation,
-}
-
 type AnnotationSummary = BTreeMap<ValidatorCode, usize>;
 
 #[derive(Default)]
@@ -77,24 +90,19 @@ impl AnnotatedTrace {
     pub fn apply_patch(&mut self, patch: AnnotatedTracePatch) -> anyhow::Result<usize> {
         let mut count = 0;
         for event in self.events.iter_mut() {
-            for (i, obj) in event
-                .data
-                .applied_objs
-                .iter_mut()
-                .chain(event.data.deleted_objs.iter_mut())
-                .enumerate()
-            {
+            for obj in event.data.applied_objs.iter_mut().chain(event.data.deleted_objs.iter_mut()) {
                 let should_apply_here = match patch.locations {
                     PatchLocations::Everywhere => true,
-                    PatchLocations::AffectedObjects(code) => event.annotations[i].contains(&code),
                     PatchLocations::ObjectReference(ref type_, ref ns_name) => {
                         obj.types.as_ref().is_some_and(|t| t == type_) && &obj.namespaced_name() == ns_name
                     },
                 };
 
                 if should_apply_here {
                     count += 1;
-                    patch_ext(&mut obj.data, patch.op.clone())?;
+                    for op in &patch.ops {
+                        patch_ext(&mut obj.data, op.clone())?;
+                    }
                 }
             }
         }
@@ -110,32 +118,36 @@ impl AnnotatedTrace {
         trace.export_all()
     }
 
-    pub fn validate(&mut self, validators: &BTreeMap<ValidatorCode, Validator>) -> AnnotationSummary {
+    pub fn validate(&mut self, validators: &BTreeMap<ValidatorCode, Validator>) -> anyhow::Result<AnnotationSummary> {
         let mut summary = BTreeMap::new();
         for event in self.events.iter_mut() {
             event.clear_annotations();
             for (code, validator) in validators.iter() {
-                let affected_indices = validator.check_next_event(event);
-                let count = affected_indices.len();
+                let event_patches = validator.check_next_event(event)?;
+                let count = event_patches.len();
                 summary.entry(*code).and_modify(|e| *e += count).or_insert(count);
 
-                for i in affected_indices {
-                    event.annotations[i].push(*code);
+                for (i, obj_patches) in event_patches {
+                    event
+                        .annotations
+                        .entry(i)
+                        .or_insert(vec![])
+                        .push(Annotation { code: *code, patches: obj_patches });
                 }
             }
         }
-        summary
+        Ok(summary)
     }
 
     pub fn get_event(&self, idx: usize) -> Option<&AnnotatedTraceEvent> {
         self.events.get(idx)
     }
 
-    pub fn get_next_error(&self) -> Option<ValidatorCode> {
+    pub fn get_next_annotation(&self) -> Option<&Annotation> {
         for event in &self.events {
-            for annotation in &event.annotations {
-                if let Some(code) = annotation.first() {
-                    return Some(*code);
+            for annotation_list in event.annotations.values() {
+                if let Some(annotation) = annotation_list.first() {
+                    return Some(annotation);
                 }
             }
         }

sk-cli/src/validation/mod.rs

@@ -1,5 +1,5 @@
 mod annotated_trace;
-mod status_field_populated;
+mod rules;
 mod summary;
 mod validation_store;
 mod validator;

sk-cli/src/validation/rules/mod.rs

@@ -0,0 +1,4 @@
+pub mod status_field_populated;
+
+#[cfg(test)]
+mod tests;

sk-cli/src/validation/status_field_populated.rs → sk-cli/src/validation/rules/status_field_populated.rs

@@ -4,25 +4,38 @@ use std::sync::{
 };
 
 use json_patch_ext::prelude::*;
+use lazy_static::lazy_static;
 
-use super::annotated_trace::AnnotatedTraceEvent;
-use super::validator::{
+use crate::validation::validator::{
+    CheckResult,
     Diagnostic,
     Validator,
     ValidatorType,
 };
+use crate::validation::{
+    AnnotatedTraceEvent,
+    AnnotatedTracePatch,
+    PatchLocations,
+};
 
 const HELP: &str = r#"Indicates that the status field of a Kubernetes object in
 the trace is non-empty; status fields are updated by their controlling objects
 and shouldn't be applied "by hand".  This is probably "fine" but it would be
 better to clean them up (and also they take up a lot of space."#;
 
 #[derive(Default)]
-pub(super) struct StatusFieldPopulated {}
+pub struct StatusFieldPopulated {}
+
+lazy_static! {
+    static ref FIX: AnnotatedTracePatch = AnnotatedTracePatch {
+        locations: PatchLocations::Everywhere,
+        ops: vec![remove_operation(format_ptr!("/status"))],
+    };
+}
 
 impl Diagnostic for StatusFieldPopulated {
-    fn check_next_event(&mut self, event: &mut AnnotatedTraceEvent) -> Vec<usize> {
-        event
+    fn check_next_event(&mut self, event: &mut AnnotatedTraceEvent) -> CheckResult {
+        Ok(event
             .data
             .applied_objs
             .iter()
@@ -35,21 +48,17 @@ impl Diagnostic for StatusFieldPopulated {
                     .get("status")
                     .is_some_and(|v| !v.is_null())
                 {
-                    return Some(i);
+                    return Some((i, vec![FIX.clone()]));
                 }
                 None
             })
-            .collect()
-    }
-
-    fn fixes(&self) -> Vec<PatchOperation> {
-        vec![remove_operation(format_ptr!("/status"))]
+            .collect())
     }
 
     fn reset(&mut self) {}
 }
 
-pub(super) fn validator() -> Validator {
+pub fn validator() -> Validator {
     Validator {
         type_: ValidatorType::Warning,
         name: "status_field_populated",

sk-cli/src/validation/rules/tests/mod.rs

@@ -0,0 +1,7 @@
+mod status_field_populated_test;
+
+use rstest::*;
+use sk_core::prelude::*;
+
+use super::*;
+use crate::validation::AnnotatedTraceEvent;

sk-cli/src/validation/tests/status_field_populated_test.rs → sk-cli/src/validation/rules/tests/status_field_populated_test.rs

@@ -15,8 +15,8 @@ fn test_status_field_populated(test_deployment: DynamicObject) {
         },
         ..Default::default()
     };
-    let indices = v.check_next_event(&mut evt);
-    assert_bag_eq!(indices, &[0]);
+    let annotations = v.check_next_event(&mut evt).unwrap();
+    assert_eq!(annotations.keys().collect::<Vec<_>>(), vec![&0]);
 }
 
 #[rstest]
@@ -30,6 +30,6 @@ fn test_status_field_not_populated(test_deployment: DynamicObject) {
         },
         ..Default::default()
     };
-    let indices = v.check_next_event(&mut evt);
-    assert_is_empty!(indices);
+    let annotations = v.check_next_event(&mut evt).unwrap();
+    assert_is_empty!(annotations);
 }

sk-cli/src/validation/tests/annotated_trace_test.rs

@@ -8,7 +8,7 @@ fn test_apply_patch_everywhere(mut annotated_trace: AnnotatedTrace) {
     annotated_trace
         .apply_patch(AnnotatedTracePatch {
             locations: PatchLocations::Everywhere,
-            op: add_operation(format_ptr!("/foo"), "bar".into()),
+            ops: vec![add_operation(format_ptr!("/foo"), "bar".into())],
         })
         .unwrap();
 
@@ -27,7 +27,7 @@ fn test_apply_patch_object_reference(mut annotated_trace: AnnotatedTrace) {
                 DEPL_GVK.into_type_meta(),
                 format!("{TEST_NAMESPACE}/test_depl1"),
             ),
-            op: add_operation(format_ptr!("/foo"), "bar".into()),
+            ops: vec![add_operation(format_ptr!("/foo"), "bar".into())],
         })
         .unwrap();
 

sk-cli/src/validation/tests/mod.rs

@@ -1,5 +1,4 @@
 mod annotated_trace_test;
-mod status_field_populated_test;
 mod validation_store_test;
 
 use std::collections::BTreeMap;
@@ -15,6 +14,7 @@ use sk_store::TraceEvent;
 
 use super::annotated_trace::AnnotatedTraceEvent;
 use super::validator::{
+    CheckResult,
     Diagnostic,
     Validator,
     ValidatorCode,
@@ -53,18 +53,20 @@ pub fn annotated_trace() -> AnnotatedTrace {
 struct TestDiagnostic {}
 
 impl Diagnostic for TestDiagnostic {
-    fn check_next_event(&mut self, evt: &mut AnnotatedTraceEvent) -> Vec<usize> {
+    fn check_next_event(&mut self, evt: &mut AnnotatedTraceEvent) -> CheckResult {
         if evt.data.applied_objs.len() > 1 && evt.data.applied_objs[1].data.get("foo").is_none() {
-            vec![1]
+            Ok(BTreeMap::from([(
+                1,
+                vec![AnnotatedTracePatch {
+                    locations: PatchLocations::Everywhere,
+                    ops: vec![add_operation(format_ptr!("/foo"), "bar".into())],
+                }],
+            )]))
         } else {
-            vec![]
+            Ok(BTreeMap::new())
         }
     }
 
-    fn fixes(&self) -> Vec<PatchOperation> {
-        vec![add_operation(format_ptr!("/foo"), "bar".into())]
-    }
-
     fn reset(&mut self) {}
 }
 

sk-cli/src/validation/tests/validation_store_test.rs

@@ -1,16 +1,17 @@
 use assertables::*;
 
 use super::*;
+use crate::validation::annotated_trace::Annotation;
 
 #[rstest]
 fn test_validate_trace(test_validation_store: ValidationStore, mut annotated_trace: AnnotatedTrace) {
     let summary = test_validation_store.validate_trace(&mut annotated_trace, false).unwrap();
 
     for event in annotated_trace.iter() {
         if event.data.applied_objs.len() > 1 {
-            assert_eq!(event.annotations[1], vec![TEST_VALIDATOR_CODE]);
+            assert_all!(event.annotations[&1].iter(), |a: &Annotation| a.code == TEST_VALIDATOR_CODE);
         } else {
-            for annotation in &event.annotations {
+            for annotation in event.annotations.values() {
                 assert_is_empty!(annotation);
             }
         }
@@ -25,15 +26,14 @@ fn test_fix_trace(test_validation_store: ValidationStore, mut annotated_trace: A
     let summary = test_validation_store.validate_trace(&mut annotated_trace, true).unwrap();
 
     for event in annotated_trace.iter() {
-        println!("{:?}", event.data);
         if event.data.applied_objs.len() > 1 {
             assert_eq!(event.data.applied_objs[1].data.get("foo").unwrap(), "bar");
         }
-        for annotation in &event.annotations {
+        for annotation in event.annotations.values() {
             assert_is_empty!(annotation);
         }
     }
 
     assert_eq!(*summary.annotations.get(&TEST_VALIDATOR_CODE).unwrap(), 1);
-    assert_eq!(summary.patches, 1);
+    assert_eq!(summary.patches, 5);
 }