Updates from PR feedback, cleanup and renaming

Signed-off-by: David Gilligan-Cook <dcook@imageworks.com>

crates/spfs-cli/common/src/args.rs

@@ -417,7 +417,7 @@ impl Logging {
     }
 }
 
-/// Command line flags for viewing annotation in a runtime
+/// Command line flags for viewing annotations in a runtime
 #[derive(Debug, Clone, clap::Args)]
 pub struct AnnotationViewing {
     /// Output the data value for the given annotation key(s) from
@@ -428,7 +428,7 @@ pub struct AnnotationViewing {
 
     /// Output all the annotation keys and values from the active
     /// runtime as a yaml dictionary
-    #[clap(long, alias = "all_annotations")]
+    #[clap(long, alias = "all-annotations")]
     pub get_all: bool,
 }
 

crates/spfs-cli/main/src/cmd_info.rs

@@ -142,7 +142,7 @@ impl CmdInfo {
                 if let Some(annotations) = obj.annotations() {
                     for data in annotations {
                         let annotation: Annotation = data.into();
-                        println!(" {}", "annotation:".bright_blue());
+                        println!(" {}", "annotations:".bright_blue());
                         println!("  {} {}", "key:".bright_blue(), annotation.key());
                         println!("  {} {}", "value:".bright_blue(), annotation.value());
                     }

crates/spfs-cli/main/src/cmd_run.rs

@@ -55,7 +55,7 @@ pub struct Annotation {
 }
 
 impl Annotation {
-    /// Returns a list of annotaion key-value pairs gathered from all
+    /// Returns a list of annotation key-value pairs gathered from all
     /// the annotation related command line arguments. The same keys,
     /// and values, can appear multiple times in the list if specified
     /// multiple times in various command line arguments.

crates/spfs/src/check.rs

@@ -342,7 +342,7 @@ where
         };
         let annotation_results = match layer.annotations() {
             None => vec![CheckObjectResult::Annotation(
-                CheckAnnotationResult::Skipped,
+                CheckAnnotationResult::InternalValue,
             )],
             Some(annotations) => {
                 let mut results = Vec::new();
@@ -353,7 +353,7 @@ where
                 }
                 if results.is_empty() {
                     results.push(CheckObjectResult::Annotation(
-                        CheckAnnotationResult::Skipped,
+                        CheckAnnotationResult::InternalValue,
                     ));
                 }
                 results
@@ -397,7 +397,7 @@ where
         annotation: graph::Annotation<'_>,
     ) -> Result<CheckAnnotationResult> {
         let res = match annotation.value() {
-            AnnotationValue::String(_) => CheckAnnotationResult::Skipped,
+            AnnotationValue::String(_) => CheckAnnotationResult::InternalValue,
             AnnotationValue::Blob(d) => {
                 let blob = self.repo.read_blob(d).await?;
                 let result = unsafe { self.check_blob(&blob).await? };
@@ -960,7 +960,7 @@ impl CheckManifestResult {
 pub enum CheckAnnotationResult {
     /// The annotation was stored directly in the layer and did not
     /// need checking
-    Skipped,
+    InternalValue,
     /// The annotation was stored in a blob and was checked
     Checked {
         digest: encoding::Digest,
@@ -978,7 +978,7 @@ impl CheckAnnotationResult {
 
     pub fn summary(&self) -> CheckSummary {
         match self {
-            Self::Skipped => CheckSummary::default(),
+            Self::InternalValue => CheckSummary::default(),
             Self::Checked { result, .. } => result.summary(),
         }
     }

crates/spfs/src/graph/layer.rs

@@ -121,30 +121,25 @@ impl Layer {
     }
 
     pub(super) fn legacy_encode(&self, mut writer: &mut impl std::io::Write) -> Result<()> {
-        tracing::trace!("layer legacy_encode called...");
         let result = if let Some(manifest_digest) = self.manifest() {
             let manifest_result =
                 encoding::write_digest(&mut writer, manifest_digest).map_err(Error::Encoding);
             if let Some(annotations) = self.annotations() {
-                tracing::trace!("layer with both manifest and annotation data");
                 for entry in annotations {
                     let annotation: Annotation = entry.into();
                     annotation.legacy_encode(&mut writer)?;
                 }
                 Ok(())
             } else {
-                tracing::trace!("layer with manifest only: {manifest_result:?}");
                 manifest_result
             }
         } else if let Some(annotations) = self.annotations() {
-            tracing::trace!("layer with annotation only");
             for entry in annotations {
                 let annotation: Annotation = entry.into();
                 annotation.legacy_encode(&mut writer)?;
             }
             Ok(())
         } else {
-            tracing::trace!("layer without either kind of data - an invalid layer");
             Err(Error::String(
                 "Invalid Layer object for legacy encoding, it has no manifest or annotation data"
                     .to_string(),

crates/spfs/src/proto/defs/types.proto

@@ -46,12 +46,6 @@ message Annotation {
     }
 }
 
-// TODO: is this needed?
-//enum AnnotationValue {
-//   STRING = 0;
-//   DIGEST = 1;
-//}
-
 message Tree {
     repeated Entry entries = 1;
 }

crates/spfs/src/runtime/storage.rs

@@ -700,46 +700,36 @@ impl Runtime {
         size_limit: usize,
     ) -> Result<()> {
         tracing::debug!(
-            "about to insert annotation key: {} => value: {} [len: {} > {}]",
+            "adding Annotation: key: {} => value: {} [len: {} > {}]",
             key,
             value,
             value.len(),
             size_limit
         );
 
         let annotation_value = if value.len() <= size_limit {
-            tracing::debug!("about to store value directly in the annotation layer");
             AnnotationValue::String(value)
         } else {
-            tracing::debug!("about to store value in a created blob");
             let digest = self.storage.create_blob_for_string(value).await?;
-            tracing::debug!("created a blob under {digest}");
+            tracing::debug!("annotation too large for Layer, created Blob for it: {digest}");
             AnnotationValue::Blob(digest)
         };
 
-        tracing::trace!("annotation_value: {annotation_value:?}");
-
         let layer = graph::Layer::new_with_annotation(key, annotation_value);
-
-        tracing::trace!("layer make with manifest: {:?}", layer.manifest());
-
         self.storage.inner.write_object(&layer).await?;
 
-        tracing::trace!("post-write_object");
-
         // The new annotation is added to the bottom of the runtime's stack
         let layer_digest = layer.digest()?;
-        tracing::trace!("layer_digest of layer with annotation: {layer_digest}");
         self.push_digest(layer_digest);
 
-        tracing::debug!("pushed layer_digest to storage, about to return: {layer_digest}.");
+        tracing::debug!("pushed layer with annotion to storage: {layer_digest}.");
         Ok(())
     }
 
     /// Return the string value stored as annotation under the given key.
     pub async fn annotation(&self, key: &str) -> Result<Option<String>> {
         for digest in self.status.stack.iter_bottom_up() {
-            if let Some(s) = self.storage.get_annotation(&digest, key).await? {
+            if let Some(s) = self.storage.find_annotation(&digest, key).await? {
                 return Ok(Some(s));
             }
         }
@@ -749,18 +739,11 @@ impl Runtime {
     /// Return all the string values that are stored as annotation under any key.
     pub async fn all_annotations(&self) -> Result<BTreeMap<String, String>> {
         let mut data: BTreeMap<String, String> = BTreeMap::new();
-        tracing::trace!(
-            "   stack: {}",
-            self.status
-                .stack
-                .iter_bottom_up()
-                .map(|d| format!("{d}"))
-                .collect::<Vec<String>>()
-                .join(", ")
-        );
         for digest in self.status.stack.iter_bottom_up() {
-            tracing::trace!("   before call get_annotation_key_value_pairs: {}", digest);
-            let pairs = self.storage.get_annotation_key_value_pairs(&digest).await?;
+            let pairs = self
+                .storage
+                .find_annotation_key_value_pairs(&digest)
+                .await?;
             for (key, value) in pairs {
                 data.insert(key, value);
             }
@@ -1236,7 +1219,7 @@ impl Storage {
     /// Returns the value from the first annotation object matching
     /// the given key that can be found starting from the given
     /// digest's spfs object.
-    pub(crate) async fn get_annotation(
+    pub(crate) async fn find_annotation(
         &self,
         digest: &Digest,
         key: &str,
@@ -1262,7 +1245,7 @@ impl Storage {
                             for entry in annotations {
                                 let annotation: Annotation = entry.into();
                                 if annotation.key() == key {
-                                    let value = self.get_annotation_value(&annotation).await?;
+                                    let value = self.find_annotation_value(&annotation).await?;
                                     return Ok(Some(value));
                                 }
                             }
@@ -1280,7 +1263,7 @@ impl Storage {
 
     /// Returns all key-value pairs from the annotation object that
     /// can be found starting from the given digest's spfs object.
-    pub(crate) async fn get_annotation_key_value_pairs(
+    pub(crate) async fn find_annotation_key_value_pairs(
         &self,
         digest: &Digest,
     ) -> Result<Vec<KeyValuePair>> {
@@ -1306,7 +1289,7 @@ impl Storage {
                             for entry in annotations {
                                 let annotation: Annotation = entry.into();
                                 let key = annotation.key().to_string();
-                                let value = self.get_annotation_value(&annotation).await?;
+                                let value = self.find_annotation_value(&annotation).await?;
                                 key_value_pairs.push((key, value));
                             }
                         }
@@ -1322,10 +1305,10 @@ impl Storage {
         Ok(key_value_pairs)
     }
 
-    /// Get the value as a string from the given annotation, loading
-    /// the value from the blob referenced by the digest if the value
-    /// is stored in an external blob.
-    async fn get_annotation_value(&self, annotation: &Annotation<'_>) -> Result<String> {
+    /// Return the value, as a string, from the given annotation,
+    /// loading the value from the blob referenced by the digest if
+    /// the value is stored in an external blob.
+    async fn find_annotation_value(&self, annotation: &Annotation<'_>) -> Result<String> {
         let data = match annotation.value() {
             AnnotationValue::String(s) => s.clone(),
             AnnotationValue::Blob(digest) => {

crates/spk-build/src/build/binary.rs

@@ -847,7 +847,7 @@ where
     let manifest_digest = match layer.manifest() {
         Some(d) => d,
         None => {
-            return Err(Error::String("Collected changes became a layer with no manifest. This should not happen during a binary build.".to_string()));
+            return Err(Error::String("Collected changes became a layer with no manifest. This should not happen during a binary build. Please report this as a bug".to_string()));
         }
     };