From eab81aee99ebb3f822369f798e550dfb8eff40ee Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 3 Mar 2026 00:59:41 -0500 Subject: [PATCH] Simplify feature fetch functions: extract shared helpers - Extract read_feature_metadata(), extract_tgz(), and create_feature_dir() helpers to eliminate copy-paste across fetch_feature_oci/local/https - Move ensure_oras() out of per-feature calls into a once-per-resolve check via oras_checked flag - Simplify dockerfile::generate() to take &HashMap instead of &Option, removing an unnecessary clone in the caller Co-Authored-By: Claude Opus 4.6 --- crates/arc-devcontainer/src/dockerfile.rs | 42 +++--- crates/arc-devcontainer/src/features.rs | 168 ++++++++-------------- crates/arc-devcontainer/src/lib.rs | 8 +- 3 files changed, 84 insertions(+), 134 deletions(-) diff --git a/crates/arc-devcontainer/src/dockerfile.rs b/crates/arc-devcontainer/src/dockerfile.rs index 6779c2b73..b4f1ab7b6 100644 --- a/crates/arc-devcontainer/src/dockerfile.rs +++ b/crates/arc-devcontainer/src/dockerfile.rs @@ -6,7 +6,7 @@ use crate::features::FeatureLayer; pub fn generate( base_dockerfile: &str, feature_layers: &[FeatureLayer], - container_env: &Option>, + container_env: &HashMap, remote_user: Option<&str>, ) -> String { let mut sections: Vec = Vec::new(); @@ -18,16 +18,14 @@ pub fn generate( sections.push(layer.dockerfile_snippet.clone()); } - if let Some(env) = container_env { - if !env.is_empty() { - let mut keys: Vec<&String> = env.keys().collect(); - keys.sort(); - let env_lines: Vec = keys - .iter() - .map(|k| format!("ENV {}={}", k, env[*k])) - .collect(); - sections.push(env_lines.join("\n")); - } + if !container_env.is_empty() { + let mut keys: Vec<&String> = container_env.keys().collect(); + keys.sort(); + let env_lines: Vec = keys + .iter() + .map(|k| format!("ENV {}={}", k, container_env[*k])) + .collect(); + sections.push(env_lines.join("\n")); } if let Some(user) = remote_user { @@ -53,7 +51,7 @@ mod tests { #[test] fn base_image_only() { - let result = generate("FROM ubuntu:22.04", &[], &None, None); + let result = generate("FROM ubuntu:22.04", &[], &HashMap::new(), None); assert_eq!( result, "# Generated by arc-devcontainer\n\nFROM ubuntu:22.04\n" @@ -63,7 +61,7 @@ mod tests { #[test] fn base_dockerfile_preserved_as_is() { let base = "FROM ubuntu:22.04\nRUN apt-get update\nRUN apt-get install -y curl"; - let result = generate(base, &[], &None, None); + let result = generate(base, &[], &HashMap::new(), None); assert_eq!( result, "# Generated by arc-devcontainer\n\nFROM ubuntu:22.04\nRUN apt-get update\nRUN apt-get install -y curl\n" @@ -76,7 +74,7 @@ mod tests { make_layer("node", "node-1", "RUN install-node.sh"), make_layer("python", "python-1", "RUN install-python.sh"), ]; - let result = generate("FROM ubuntu:22.04", &layers, &None, None); + let result = generate("FROM ubuntu:22.04", &layers, &HashMap::new(), None); assert_eq!( result, "# Generated by arc-devcontainer\n\n\ @@ -92,7 +90,7 @@ mod tests { env.insert("ZEBRA".to_string(), "stripes".to_string()); env.insert("APPLE".to_string(), "red".to_string()); env.insert("MANGO".to_string(), "yellow".to_string()); - let result = generate("FROM alpine", &[], &Some(env), None); + let result = generate("FROM alpine", &[], &env, None); assert_eq!( result, "# Generated by arc-devcontainer\n\n\ @@ -105,7 +103,7 @@ mod tests { #[test] fn with_remote_user() { - let result = generate("FROM alpine", &[], &None, Some("vscode")); + let result = generate("FROM alpine", &[], &HashMap::new(), Some("vscode")); assert_eq!( result, "# Generated by arc-devcontainer\n\n\ @@ -120,7 +118,7 @@ mod tests { let mut env = HashMap::new(); env.insert("PATH".to_string(), "/usr/local/bin".to_string()); env.insert("HOME".to_string(), "/home/vscode".to_string()); - let result = generate("FROM ubuntu:22.04", &layers, &Some(env), Some("vscode")); + let result = generate("FROM ubuntu:22.04", &layers, &env, Some("vscode")); assert_eq!( result, "# Generated by arc-devcontainer\n\n\ @@ -134,7 +132,7 @@ mod tests { #[test] fn empty_feature_layers_no_extra_blank_lines() { - let result = generate("FROM alpine", &[], &None, Some("dev")); + let result = generate("FROM alpine", &[], &HashMap::new(), Some("dev")); assert_eq!( result, "# Generated by arc-devcontainer\n\n\ @@ -146,7 +144,7 @@ mod tests { #[test] fn empty_env_map_treated_as_none() { let env = HashMap::new(); - let result = generate("FROM alpine", &[], &Some(env), None); + let result = generate("FROM alpine", &[], &env, None); assert_eq!( result, "# Generated by arc-devcontainer\n\nFROM alpine\n" @@ -162,7 +160,7 @@ mod tests { \n\ FROM ubuntu:22.04\n\ COPY --from=builder /app/bin /usr/local/bin"; - let result = generate(base, &[], &None, None); + let result = generate(base, &[], &HashMap::new(), None); assert!(result.starts_with("# Generated by arc-devcontainer\n\n")); assert!(result.contains("FROM ubuntu:22.04 AS builder")); assert!(result.contains("COPY --from=builder /app/bin /usr/local/bin")); @@ -173,7 +171,7 @@ mod tests { fn container_env_only() { let mut cenv = HashMap::new(); cenv.insert("DEBIAN_FRONTEND".to_string(), "noninteractive".to_string()); - let result = generate("FROM alpine", &[], &Some(cenv), None); + let result = generate("FROM alpine", &[], &cenv, None); assert_eq!( result, "# Generated by arc-devcontainer\n\n\ @@ -188,7 +186,7 @@ mod tests { cenv.insert("ALPHA".to_string(), "first".to_string()); cenv.insert("BETA".to_string(), "second".to_string()); cenv.insert("GAMMA".to_string(), "third".to_string()); - let result = generate("FROM alpine", &[], &Some(cenv), None); + let result = generate("FROM alpine", &[], &cenv, None); assert!(result.contains("ENV ALPHA=first")); assert!(result.contains("ENV BETA=second")); assert!(result.contains("ENV GAMMA=third")); diff --git a/crates/arc-devcontainer/src/features.rs b/crates/arc-devcontainer/src/features.rs index 51770b66c..39f505eab 100644 --- a/crates/arc-devcontainer/src/features.rs +++ b/crates/arc-devcontainer/src/features.rs @@ -132,14 +132,45 @@ async fn ensure_oras() -> crate::Result<()> { Ok(()) } -/// Fetch a single OCI feature using `oras pull` and extract its contents. -/// Returns the parsed feature metadata. -async fn fetch_feature_oci( - feature_id: &str, - output_dir: &Path, -) -> crate::Result { - ensure_oras().await?; +/// Extract a tgz archive in the given directory. +async fn extract_tgz(feature_dir: &Path, feature_id: &str) -> crate::Result<()> { + let status = tokio::process::Command::new("tar") + .args(["xzf", "devcontainer-feature.tgz"]) + .current_dir(feature_dir) + .status() + .await + .map_err(|e| DevcontainerError::Feature(format!("failed to extract tgz: {e}")))?; + if !status.success() { + return Err(DevcontainerError::Feature(format!( + "tar extraction failed for {feature_id}" + ))); + } + Ok(()) +} + +/// Read and parse devcontainer-feature.json from a feature directory. +async fn read_feature_metadata(feature_dir: &Path) -> crate::Result { + let metadata_path = feature_dir.join("devcontainer-feature.json"); + let metadata_str = tokio::fs::read_to_string(&metadata_path) + .await + .map_err(|e| { + DevcontainerError::Feature(format!( + "failed to read {}: {e}", + metadata_path.display() + )) + })?; + + serde_json::from_str(&metadata_str).map_err(|e| { + DevcontainerError::Feature(format!( + "failed to parse {}: {e}", + metadata_path.display() + )) + }) +} + +/// Create a feature output directory under the temp dir. +async fn create_feature_dir(output_dir: &Path, feature_id: &str) -> crate::Result { let dir_name = dir_name_from_id(feature_id); let feature_dir = output_dir.join(&dir_name); tokio::fs::create_dir_all(&feature_dir) @@ -150,6 +181,15 @@ async fn fetch_feature_oci( feature_dir.display() )) })?; + Ok(feature_dir) +} + +/// Fetch a single OCI feature using `oras pull` and extract its contents. +async fn fetch_feature_oci( + feature_id: &str, + output_dir: &Path, +) -> crate::Result { + let feature_dir = create_feature_dir(output_dir, feature_id).await?; info!(feature_id, "pulling feature with oras"); @@ -167,44 +207,11 @@ async fn fetch_feature_oci( ))); } - // Extract any tgz files - let tgz_path = feature_dir.join("devcontainer-feature.tgz"); - if tgz_path.exists() { - let status = tokio::process::Command::new("tar") - .args(["xzf", "devcontainer-feature.tgz"]) - .current_dir(&feature_dir) - .status() - .await - .map_err(|e| { - DevcontainerError::Feature(format!("failed to extract tgz: {e}")) - })?; - - if !status.success() { - return Err(DevcontainerError::Feature(format!( - "tar extraction failed for {feature_id}" - ))); - } + if feature_dir.join("devcontainer-feature.tgz").exists() { + extract_tgz(&feature_dir, feature_id).await?; } - // Read metadata - let metadata_path = feature_dir.join("devcontainer-feature.json"); - let metadata_str = tokio::fs::read_to_string(&metadata_path) - .await - .map_err(|e| { - DevcontainerError::Feature(format!( - "failed to read {}: {e}", - metadata_path.display() - )) - })?; - - let metadata: FeatureMetadata = serde_json::from_str(&metadata_str).map_err(|e| { - DevcontainerError::Feature(format!( - "failed to parse {}: {e}", - metadata_path.display() - )) - })?; - - Ok(metadata) + read_feature_metadata(&feature_dir).await } /// Fetch a local feature by copying its directory. @@ -221,28 +228,10 @@ async fn fetch_feature_local( ))); } - let dir_name = dir_name_from_id(feature_id); - let feature_dir = output_dir.join(&dir_name); + let feature_dir = create_feature_dir(output_dir, feature_id).await?; copy_dir_recursive(&local_path, &feature_dir).await?; - let metadata_path = feature_dir.join("devcontainer-feature.json"); - let metadata_str = tokio::fs::read_to_string(&metadata_path) - .await - .map_err(|e| { - DevcontainerError::Feature(format!( - "failed to read {}: {e}", - metadata_path.display() - )) - })?; - - let metadata: FeatureMetadata = serde_json::from_str(&metadata_str).map_err(|e| { - DevcontainerError::Feature(format!( - "failed to parse {}: {e}", - metadata_path.display() - )) - })?; - - Ok(metadata) + read_feature_metadata(&feature_dir).await } /// Fetch a feature from an HTTPS URL (tgz archive). @@ -250,16 +239,7 @@ async fn fetch_feature_https( feature_id: &str, output_dir: &Path, ) -> crate::Result { - let dir_name = dir_name_from_id(feature_id); - let feature_dir = output_dir.join(&dir_name); - tokio::fs::create_dir_all(&feature_dir) - .await - .map_err(|e| { - DevcontainerError::Feature(format!( - "failed to create dir {}: {e}", - feature_dir.display() - )) - })?; + let feature_dir = create_feature_dir(output_dir, feature_id).await?; info!(feature_id, "downloading feature from HTTPS"); @@ -286,37 +266,9 @@ async fn fetch_feature_https( )) })?; - let status = tokio::process::Command::new("tar") - .args(["xzf", "devcontainer-feature.tgz"]) - .current_dir(&feature_dir) - .status() - .await - .map_err(|e| DevcontainerError::Feature(format!("failed to extract tgz: {e}")))?; + extract_tgz(&feature_dir, feature_id).await?; - if !status.success() { - return Err(DevcontainerError::Feature(format!( - "tar extraction failed for {feature_id}" - ))); - } - - let metadata_path = feature_dir.join("devcontainer-feature.json"); - let metadata_str = tokio::fs::read_to_string(&metadata_path) - .await - .map_err(|e| { - DevcontainerError::Feature(format!( - "failed to read {}: {e}", - metadata_path.display() - )) - })?; - - let metadata: FeatureMetadata = serde_json::from_str(&metadata_str).map_err(|e| { - DevcontainerError::Feature(format!( - "failed to parse {}: {e}", - metadata_path.display() - )) - })?; - - Ok(metadata) + read_feature_metadata(&feature_dir).await } /// Dispatch feature fetch based on the feature ID prefix. @@ -324,12 +276,17 @@ async fn fetch_feature_dispatch( feature_id: &str, output_dir: &Path, devcontainer_dir: &Path, + oras_checked: &mut bool, ) -> crate::Result { if feature_id.starts_with("./") || feature_id.starts_with("../") { fetch_feature_local(feature_id, output_dir, devcontainer_dir).await } else if feature_id.starts_with("https://") { fetch_feature_https(feature_id, output_dir).await } else { + if !*oras_checked { + ensure_oras().await?; + *oras_checked = true; + } fetch_feature_oci(feature_id, output_dir).await } } @@ -560,9 +517,10 @@ pub async fn resolve_features( let mut all_options: HashMap = features.clone(); // Fetch all features and collect metadata + let mut oras_checked = false; let mut metadata_map: HashMap = HashMap::new(); for feature_id in &feature_ids { - let metadata = fetch_feature_dispatch(feature_id, &tmp_dir, devcontainer_dir).await?; + let metadata = fetch_feature_dispatch(feature_id, &tmp_dir, devcontainer_dir, &mut oras_checked).await?; metadata_map.insert(feature_id.clone(), metadata); } @@ -581,7 +539,7 @@ pub async fn resolve_features( }); if !already_present { info!(dep_id, "auto-injecting missing dependsOn target"); - let dep_metadata = fetch_feature_dispatch(dep_id, &tmp_dir, devcontainer_dir).await?; + let dep_metadata = fetch_feature_dispatch(dep_id, &tmp_dir, devcontainer_dir, &mut oras_checked).await?; metadata_map.insert(dep_id.clone(), dep_metadata); feature_ids.push(dep_id.clone()); all_options.insert(dep_id.clone(), dep_options.clone()); diff --git a/crates/arc-devcontainer/src/lib.rs b/crates/arc-devcontainer/src/lib.rs index 009ba940c..a02d5757e 100644 --- a/crates/arc-devcontainer/src/lib.rs +++ b/crates/arc-devcontainer/src/lib.rs @@ -262,17 +262,11 @@ impl DevcontainerResolver { merged_container_env.insert(k.clone(), variables::substitute(v, &vars)); } } - let container_env_option = if merged_container_env.is_empty() { - None - } else { - Some(merged_container_env.clone()) - }; - // Generate final Dockerfile let dockerfile_content = dockerfile::generate( &base_dockerfile, &resolved_features.layers, - &container_env_option, + &merged_container_env, devcontainer.remote_user.as_deref(), );