Merge pull request #1980 from fermyon/rejigger-spin-up

Fix `spin up --help` for unbuilt apps

Author: lann

src/commands/up.rs

@@ -1,11 +1,12 @@
+mod app_source;
+
 use std::{
-    collections::HashSet,
     ffi::OsString,
-    fmt::{Debug, Display},
+    fmt::Debug,
     path::{Path, PathBuf},
 };
 
-use anyhow::{anyhow, bail, ensure, Context, Result};
+use anyhow::{anyhow, bail, Context, Result};
 use clap::{CommandFactory, Parser};
 use reqwest::Url;
 use spin_app::locked::LockedApp;
@@ -16,6 +17,8 @@ use tempfile::TempDir;
 
 use crate::opts::*;
 
+use self::app_source::{AppSource, ResolvedAppSource};
+
 const APPLICATION_OPT: &str = "APPLICATION";
 
 /// Start the Fermyon runtime.
@@ -120,7 +123,7 @@ impl UpCommand {
     }
 
     async fn run_inner(self) -> Result<()> {
-        let app_source = self.resolve_app_source();
+        let app_source = self.app_source();
 
         if app_source == AppSource::None {
             if self.help {
@@ -144,20 +147,19 @@ impl UpCommand {
             .canonicalize()
             .context("Could not canonicalize working directory")?;
 
-        let mut locked_app = match &app_source {
-            AppSource::None => bail!("Internal error - should have shown help"),
-            AppSource::File(path) => self.prepare_app_from_file(path, &working_dir).await?,
-            AppSource::OciRegistry(oci) => self.prepare_app_from_oci(oci, &working_dir).await?,
-            AppSource::Unresolvable(err) => bail!("{err}"),
-        };
+        let resolved_app_source = self.resolve_app_source(&app_source, &working_dir).await?;
 
-        let trigger_cmd = trigger_command_from_locked_app(&locked_app)
+        let trigger_cmd = trigger_command_for_resolved_app_source(&resolved_app_source)
             .with_context(|| format!("Couldn't find trigger executor for {app_source}"))?;
 
         if self.help {
             return self.run_trigger(trigger_cmd, None).await;
         }
 
+        let mut locked_app = self
+            .load_resolved_app_source(resolved_app_source, &working_dir)
+            .await?;
+
         self.update_locked_app(&mut locked_app);
 
         let local_app_dir = app_source.local_app_dir().map(Into::into);
@@ -240,11 +242,11 @@ impl UpCommand {
         }
     }
 
-    fn resolve_app_source(&self) -> AppSource {
+    fn app_source(&self) -> AppSource {
         match (&self.app_source, &self.file_source, &self.registry_source) {
             (None, None, None) => self.default_manifest_or_none(),
-            (Some(source), None, None) => Self::infer_source(source),
-            (None, Some(file), None) => Self::infer_file_source(file.to_owned()),
+            (Some(source), None, None) => AppSource::infer_source(source),
+            (None, Some(file), None) => AppSource::infer_file_source(file.to_owned()),
             (None, None, Some(reference)) => AppSource::OciRegistry(reference.to_owned()),
             _ => AppSource::unresolvable("More than one application source was specified"),
         }
@@ -265,24 +267,6 @@ impl UpCommand {
         }
     }
 
-    fn infer_source(source: &str) -> AppSource {
-        let path = PathBuf::from(source);
-        if path.exists() {
-            Self::infer_file_source(path)
-        } else if spin_oci::is_probably_oci_reference(source) {
-            AppSource::OciRegistry(source.to_owned())
-        } else {
-            AppSource::Unresolvable(format!("File or directory '{source}' not found. If you meant to load from a registry, use the `--from-registry` option."))
-        }
-    }
-
-    fn infer_file_source(path: impl Into<PathBuf>) -> AppSource {
-        match spin_common::paths::resolve_manifest_file_path(path.into()) {
-            Ok(file) => AppSource::File(file),
-            Err(e) => AppSource::Unresolvable(e.to_string()),
-        }
-    }
-
     fn trigger_args_look_file_like(&self) -> bool {
         // Heuristic for the user typing `spin up foo` instead of `spin up -f foo` - in the
         // first case `foo` gets interpreted as a trigger arg which is probably not what the
@@ -308,29 +292,54 @@ impl UpCommand {
         Ok(locked_url)
     }
 
-    async fn prepare_app_from_file(
+    // Take the AppSource and do the minimum amount of work necessary to
+    // be able to resolve the trigger executor.
+    async fn resolve_app_source(
         &self,
-        manifest_path: &Path,
+        app_source: &AppSource,
         working_dir: &Path,
-    ) -> Result<LockedApp> {
-        let files_mount_strategy = if self.direct_mounts {
-            FilesMountStrategy::Direct
-        } else {
-            FilesMountStrategy::Copy(working_dir.join("assets"))
-        };
-        spin_loader::from_file(manifest_path, files_mount_strategy)
-            .await
-            .with_context(|| format!("Failed to load manifest from {manifest_path:?}"))
+    ) -> anyhow::Result<ResolvedAppSource> {
+        Ok(match &app_source {
+            AppSource::File(path) => ResolvedAppSource::File {
+                manifest_path: path.clone(),
+                manifest: spin_manifest::manifest_from_file(path)?,
+            },
+            // TODO: We could make the `--help` experience a little faster if
+            // we could fetch just the locked app JSON at this stage.
+            AppSource::OciRegistry(reference) => {
+                let mut client = spin_oci::Client::new(self.insecure, None)
+                    .await
+                    .context("cannot create registry client")?;
+
+                let locked_app = OciLoader::new(working_dir)
+                    .load_app(&mut client, reference)
+                    .await?;
+                ResolvedAppSource::OciRegistry { locked_app }
+            }
+            AppSource::Unresolvable(err) => bail!("{err}"),
+            AppSource::None => bail!("Internal error - should have shown help"),
+        })
     }
 
-    async fn prepare_app_from_oci(&self, reference: &str, working_dir: &Path) -> Result<LockedApp> {
-        let mut client = spin_oci::Client::new(self.insecure, None)
-            .await
-            .context("cannot create registry client")?;
-
-        OciLoader::new(working_dir)
-            .load_app(&mut client, reference)
-            .await
+    // Finish preparing a ResolvedAppSource for execution.
+    async fn load_resolved_app_source(
+        &self,
+        resolved: ResolvedAppSource,
+        working_dir: &Path,
+    ) -> anyhow::Result<LockedApp> {
+        match resolved {
+            ResolvedAppSource::File { manifest_path, .. } => {
+                let files_mount_strategy = if self.direct_mounts {
+                    FilesMountStrategy::Direct
+                } else {
+                    FilesMountStrategy::Copy(working_dir.join("assets"))
+                };
+                spin_loader::from_file(&manifest_path, files_mount_strategy)
+                    .await
+                    .with_context(|| format!("Failed to load manifest from {manifest_path:?}"))
+            }
+            ResolvedAppSource::OciRegistry { locked_app } => Ok(locked_app),
+        }
     }
 
     fn update_locked_app(&self, locked_app: &mut LockedApp) {
@@ -409,17 +418,8 @@ fn trigger_command(trigger_type: &str) -> Vec<String> {
     vec!["trigger".to_owned(), trigger_type.to_owned()]
 }
 
-fn trigger_command_from_locked_app(locked_app: &LockedApp) -> Result<Vec<String>> {
-    let trigger_type = {
-        let types = locked_app
-            .triggers
-            .iter()
-            .map(|t| t.trigger_type.as_str())
-            .collect::<HashSet<_>>();
-        ensure!(!types.is_empty(), "no triggers in app");
-        ensure!(types.len() == 1, "multiple trigger types not yet supported");
-        types.into_iter().next().unwrap()
-    };
+fn trigger_command_for_resolved_app_source(resolved: &ResolvedAppSource) -> Result<Vec<String>> {
+    let trigger_type = resolved.trigger_type()?;
 
     match trigger_type {
         "http" | "redis" => Ok(trigger_command(trigger_type)),
@@ -430,50 +430,10 @@ fn trigger_command_from_locked_app(locked_app: &LockedApp) -> Result<Vec<String>
     }
 }
 
-#[derive(Debug, PartialEq, Eq)]
-enum AppSource {
-    None,
-    File(PathBuf),
-    OciRegistry(String),
-    Unresolvable(String),
-}
-
-impl AppSource {
-    fn unresolvable(message: impl Into<String>) -> Self {
-        Self::Unresolvable(message.into())
-    }
-
-    fn local_app_dir(&self) -> Option<&Path> {
-        match self {
-            Self::File(path) => path.parent().or_else(|| {
-                tracing::warn!("Error finding local app dir from source {path:?}");
-                None
-            }),
-            _ => None,
-        }
-    }
-
-    async fn build(&self) -> anyhow::Result<()> {
-        match self {
-            Self::File(path) => spin_build::build(path, &[]).await,
-            _ => Ok(()),
-        }
-    }
-}
-
-impl Display for AppSource {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        match self {
-            AppSource::None => write!(f, "<no source>"),
-            AppSource::File(path) => write!(f, "local app {path:?}"),
-            AppSource::OciRegistry(reference) => write!(f, "remote app {reference:?}"),
-            AppSource::Unresolvable(s) => write!(f, "unknown app source: {s:?}"),
-        }
-    }
-}
-
 #[cfg(test)]
 mod test {
+    use crate::commands::up::app_source::AppSource;
+
     use super::*;
 
     fn repo_path(path: &str) -> String {
@@ -490,7 +450,7 @@ mod test {
             app_source: Some(file.clone()),
             ..Default::default()
         }
-        .resolve_app_source();
+        .app_source();
 
         assert_eq!(AppSource::File(PathBuf::from(file)), source);
     }
@@ -503,7 +463,7 @@ mod test {
             app_source: Some(dir.clone()),
             ..Default::default()
         }
-        .resolve_app_source();
+        .app_source();
 
         assert_eq!(
             AppSource::File(PathBuf::from(dir).join("spin.toml")),
@@ -519,7 +479,7 @@ mod test {
             app_source: Some(file),
             ..Default::default()
         }
-        .resolve_app_source();
+        .app_source();
 
         assert!(matches!(source, AppSource::Unresolvable(_)));
     }
@@ -532,7 +492,7 @@ mod test {
             app_source: Some(file),
             ..Default::default()
         }
-        .resolve_app_source();
+        .app_source();
 
         assert!(matches!(source, AppSource::Unresolvable(_)));
     }
@@ -545,7 +505,7 @@ mod test {
             app_source: Some(dir),
             ..Default::default()
         }
-        .resolve_app_source();
+        .app_source();
 
         assert!(matches!(source, AppSource::Unresolvable(_)));
     }
@@ -558,7 +518,7 @@ mod test {
             app_source: Some(reference.clone()),
             ..Default::default()
         }
-        .resolve_app_source();
+        .app_source();
 
         assert_eq!(AppSource::OciRegistry(reference), source);
     }
@@ -572,7 +532,7 @@ mod test {
             app_source: Some(reference.clone()),
             ..Default::default()
         }
-        .resolve_app_source();
+        .app_source();
 
         assert_eq!(AppSource::OciRegistry(reference), source);
     }
@@ -585,7 +545,7 @@ mod test {
             app_source: Some(garbage),
             ..Default::default()
         }
-        .resolve_app_source();
+        .app_source();
 
         // Honestly I feel Unresolvable might be a bit weak sauce for this case
         assert!(matches!(source, AppSource::Unresolvable(_)));