From 66f9aef3a2e3b1fa23b538b294f279596a1c064f Mon Sep 17 00:00:00 2001 From: dcookspi <92065525+dcookspi@users.noreply.github.com> Date: Thu, 29 Feb 2024 16:23:37 -0800 Subject: [PATCH] Updates from PR feedback, cleanup and renaming Signed-off-by: David Gilligan-Cook --- crates/spfs-cli/common/src/args.rs | 4 +-- crates/spfs-cli/main/src/cmd_info.rs | 2 +- crates/spfs-cli/main/src/cmd_run.rs | 2 +- crates/spfs/src/check.rs | 10 +++--- crates/spfs/src/graph/layer.rs | 5 --- crates/spfs/src/proto/defs/types.proto | 6 ---- crates/spfs/src/runtime/storage.rs | 49 +++++++++----------------- crates/spk-build/src/build/binary.rs | 2 +- 8 files changed, 26 insertions(+), 54 deletions(-) diff --git a/crates/spfs-cli/common/src/args.rs b/crates/spfs-cli/common/src/args.rs index 8d7adc4c8..9e92a11e7 100644 --- a/crates/spfs-cli/common/src/args.rs +++ b/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, } diff --git a/crates/spfs-cli/main/src/cmd_info.rs b/crates/spfs-cli/main/src/cmd_info.rs index df325ddb6..f7e1a6a01 100644 --- a/crates/spfs-cli/main/src/cmd_info.rs +++ b/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()); } diff --git a/crates/spfs-cli/main/src/cmd_run.rs b/crates/spfs-cli/main/src/cmd_run.rs index 6c99dbe7e..b369b6071 100644 --- a/crates/spfs-cli/main/src/cmd_run.rs +++ b/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. diff --git a/crates/spfs/src/check.rs b/crates/spfs/src/check.rs index 9e815fff8..7efbc1d61 100644 --- a/crates/spfs/src/check.rs +++ b/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 { 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(), } } diff --git a/crates/spfs/src/graph/layer.rs b/crates/spfs/src/graph/layer.rs index 73c0cd208..80c183b1a 100644 --- a/crates/spfs/src/graph/layer.rs +++ b/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(), diff --git a/crates/spfs/src/proto/defs/types.proto b/crates/spfs/src/proto/defs/types.proto index f9fb9f9df..b7498f097 100644 --- a/crates/spfs/src/proto/defs/types.proto +++ b/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; } diff --git a/crates/spfs/src/runtime/storage.rs b/crates/spfs/src/runtime/storage.rs index 0ff374a58..0dff55524 100644 --- a/crates/spfs/src/runtime/storage.rs +++ b/crates/spfs/src/runtime/storage.rs @@ -700,7 +700,7 @@ impl Runtime { size_limit: usize, ) -> Result<()> { tracing::debug!( - "about to insert annotation key: {} => value: {} [len: {} > {}]", + "adding Annotation: key: {} => value: {} [len: {} > {}]", key, value, value.len(), @@ -708,38 +708,28 @@ impl Runtime { ); 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> { 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> { let mut data: BTreeMap = BTreeMap::new(); - tracing::trace!( - " stack: {}", - self.status - .stack - .iter_bottom_up() - .map(|d| format!("{d}")) - .collect::>() - .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> { @@ -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 { + /// 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 { let data = match annotation.value() { AnnotationValue::String(s) => s.clone(), AnnotationValue::Blob(digest) => { diff --git a/crates/spk-build/src/build/binary.rs b/crates/spk-build/src/build/binary.rs index 03bf1ea24..71214d581 100644 --- a/crates/spk-build/src/build/binary.rs +++ b/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())); } };