Separate common trigger options from Factors specific ones.

Signed-off-by: Ryan Levick <[email protected]>

Author: rylev

crates/trigger/src/cli.rs

@@ -3,14 +3,14 @@ mod sqlite_statements;
 mod summary;
 
 use std::future::Future;
-use std::path::{Path, PathBuf};
+use std::path::PathBuf;
 
 use anyhow::{Context, Result};
 use clap::{Args, IntoApp, Parser};
 use spin_app::App;
+use spin_common::sloth;
 use spin_common::ui::quoted_path;
 use spin_common::url::parse_file_url;
-use spin_common::{arg_parser::parse_kv, sloth};
 use spin_core::async_trait;
 use spin_factors::RuntimeFactors;
 use spin_factors_executor::{ComponentLoader, FactorsExecutor};
@@ -20,7 +20,6 @@ use summary::{
     summarize_runtime_config, KeyValueDefaultStoreSummaryHook, SqliteDefaultStoreSummaryHook,
 };
 
-use crate::factors::{TriggerFactors, TriggerFactorsRuntimeConfig};
 use crate::stdio::{FollowComponents, StdioLoggingExecutorHooks};
 use crate::{Trigger, TriggerApp};
 pub use launch_metadata::LaunchMetadata;
@@ -40,9 +39,9 @@ pub const SPIN_WORKING_DIR: &str = "SPIN_WORKING_DIR";
 #[derive(Parser, Debug)]
 #[clap(
     usage = "spin [COMMAND] [OPTIONS]",
-    next_help_heading = help_heading::<T, F>()
+    next_help_heading = help_heading::<T, B::Factors>()
 )]
-pub struct FactorsTriggerCommand<T: Trigger<F>, F: RuntimeFactors> {
+pub struct FactorsTriggerCommand<T: Trigger<B::Factors>, B: RuntimeFactorsBuilder> {
     /// Log directory for the stdout and stderr of components. Setting to
     /// the empty string disables logging to disk.
     #[clap(
@@ -117,16 +116,8 @@ pub struct FactorsTriggerCommand<T: Trigger<F>, F: RuntimeFactors> {
     #[clap(flatten)]
     pub trigger_args: T::CliArgs,
 
-    /// Set a key/value pair (key=value) in the application's
-    /// default store. Any existing value will be overwritten.
-    /// Can be used multiple times.
-    #[clap(long = "key-value", parse(try_from_str = parse_kv))]
-    key_values: Vec<(String, String)>,
-
-    /// Run a SQLite statement such as a migration against the default database.
-    /// To run from a file, prefix the filename with @ e.g. spin up --sqlite @migration.sql
-    #[clap(long = "sqlite")]
-    sqlite_statements: Vec<String>,
+    #[clap(flatten)]
+    pub builder_args: B::Options,
 
     #[clap(long = "help-args-only", hide = true)]
     pub help_args_only: bool,
@@ -135,16 +126,33 @@ pub struct FactorsTriggerCommand<T: Trigger<F>, F: RuntimeFactors> {
     pub launch_metadata_only: bool,
 }
 
+/// Configuration options that are common to all triggers.
+#[derive(Debug)]
+pub struct CommonTriggerOptions {
+    /// The Spin working directory.
+    pub working_dir: PathBuf,
+    /// Path to the runtime config file.
+    pub runtime_config_file: Option<PathBuf>,
+    /// Path to the state directory.
+    pub state_dir: Option<String>,
+    /// Path to the local app directory.
+    pub local_app_dir: Option<String>,
+    /// Whether to allow transient writes to mounted files
+    pub allow_transient_write: bool,
+    /// Which components should have their logs followed.
+    pub follow_components: FollowComponents,
+    /// Log directory for component stdout/stderr.
+    pub log_dir: Option<PathBuf>,
+}
+
 /// An empty implementation of clap::Args to be used as TriggerExecutor::RunConfig
 /// for executors that do not need additional CLI args.
 #[derive(Args)]
 pub struct NoCliArgs;
 
-impl<T: Trigger<F>, F: RuntimeFactors> FactorsTriggerCommand<T, F> {
+impl<T: Trigger<B::Factors>, B: RuntimeFactorsBuilder> FactorsTriggerCommand<T, B> {
     /// Create a new TriggerExecutorBuilder from this TriggerExecutorCommand.
-    pub async fn run<B: RuntimeFactorsBuilder<Factors = F, Options = TriggerAppOptions>>(
-        self,
-    ) -> Result<()> {
+    pub async fn run(self) -> Result<()> {
         // Handle --help-args-only
         if self.help_args_only {
             Self::command()
@@ -156,7 +164,7 @@ impl<T: Trigger<F>, F: RuntimeFactors> FactorsTriggerCommand<T, F> {
 
         // Handle --launch-metadata-only
         if self.launch_metadata_only {
-            let lm = LaunchMetadata::infer::<T, F>();
+            let lm = LaunchMetadata::infer::<T, B>();
             let json = serde_json::to_string_pretty(&lm)?;
             eprintln!("{json}");
             return Ok(());
@@ -185,8 +193,7 @@ impl<T: Trigger<F>, F: RuntimeFactors> FactorsTriggerCommand<T, F> {
         }
 
         let trigger = T::new(self.trigger_args, &app)?;
-        let mut builder: TriggerAppBuilder<T, B> =
-            TriggerAppBuilder::new(trigger, PathBuf::from(working_dir));
+        let mut builder: TriggerAppBuilder<T, B> = TriggerAppBuilder::new(trigger);
         let config = builder.engine_config();
 
         // Apply --cache / --disable-cache
@@ -198,21 +205,16 @@ impl<T: Trigger<F>, F: RuntimeFactors> FactorsTriggerCommand<T, F> {
             config.disable_pooling();
         }
 
-        let run_fut = builder
-            .run(
-                app,
-                TriggerAppOptions {
-                    runtime_config_file: self.runtime_config_file.clone(),
-                    state_dir: self.state_dir.clone(),
-                    local_app_dir: local_app_dir.clone(),
-                    initial_key_values: self.key_values,
-                    sqlite_statements: self.sqlite_statements,
-                    allow_transient_write: self.allow_transient_write,
-                    follow_components,
-                    log_dir: self.log,
-                },
-            )
-            .await?;
+        let common_options = CommonTriggerOptions {
+            working_dir: PathBuf::from(working_dir),
+            runtime_config_file: self.runtime_config_file.clone(),
+            state_dir: self.state_dir.clone(),
+            local_app_dir: local_app_dir.clone(),
+            allow_transient_write: self.allow_transient_write,
+            follow_components,
+            log_dir: self.log,
+        };
+        let run_fut = builder.run(app, common_options, self.builder_args).await?;
 
         let (abortable, abort_handle) = futures::future::abortable(run_fut);
         ctrlc::set_handler(move || abort_handle.abort())?;
@@ -271,37 +273,14 @@ fn help_heading<T: Trigger<F>, F: RuntimeFactors>() -> Option<&'static str> {
 /// A builder for a [`TriggerApp`].
 pub struct TriggerAppBuilder<T, F> {
     engine_config: spin_core::Config,
-    working_dir: PathBuf,
     pub trigger: T,
     _factors: std::marker::PhantomData<F>,
 }
 
-/// Options for building a [`TriggerApp`].
-#[derive(Default)]
-pub struct TriggerAppOptions {
-    /// Path to the runtime config file.
-    pub runtime_config_file: Option<PathBuf>,
-    /// Path to the state directory.
-    pub state_dir: Option<String>,
-    /// Path to the local app directory.
-    pub local_app_dir: Option<String>,
-    /// Initial key/value pairs to set in the app's default store.
-    pub initial_key_values: Vec<(String, String)>,
-    /// SQLite statements to run.
-    pub sqlite_statements: Vec<String>,
-    /// Whether to allow transient writes to mounted files
-    pub allow_transient_write: bool,
-    /// Which components should have their logs followed.
-    pub follow_components: FollowComponents,
-    /// Log directory for component stdout/stderr.
-    pub log_dir: Option<PathBuf>,
-}
-
-impl<T: Trigger<F::Factors>, F: RuntimeFactorsBuilder> TriggerAppBuilder<T, F> {
-    pub fn new(trigger: T, working_dir: PathBuf) -> Self {
+impl<T: Trigger<B::Factors>, B: RuntimeFactorsBuilder> TriggerAppBuilder<T, B> {
+    pub fn new(trigger: T) -> Self {
         Self {
             engine_config: spin_core::Config::default(),
-            working_dir,
             trigger,
             _factors: Default::default(),
         }
@@ -315,16 +294,17 @@ impl<T: Trigger<F::Factors>, F: RuntimeFactorsBuilder> TriggerAppBuilder<T, F> {
     pub async fn build(
         &mut self,
         app: App,
-        options: F::Options,
-    ) -> anyhow::Result<TriggerApp<T, F::Factors>> {
+        common_options: CommonTriggerOptions,
+        options: B::Options,
+    ) -> anyhow::Result<TriggerApp<T, B::Factors>> {
         let mut core_engine_builder = {
             self.trigger.update_core_config(&mut self.engine_config)?;
 
             spin_core::Engine::builder(&self.engine_config)?
         };
         self.trigger.add_to_linker(core_engine_builder.linker())?;
 
-        let (factors, runtime_config) = F::new().build(self.working_dir.clone(), options)?;
+        let (factors, runtime_config) = B::build(common_options, options)?;
 
         // TODO: port the rest of the component loader logic
         struct SimpleComponentLoader;
@@ -409,24 +389,20 @@ impl<T: Trigger<F::Factors>, F: RuntimeFactorsBuilder> TriggerAppBuilder<T, F> {
     pub async fn run(
         mut self,
         app: App,
-        options: F::Options,
+        common_options: CommonTriggerOptions,
+        options: B::Options,
     ) -> anyhow::Result<impl Future<Output = anyhow::Result<()>>> {
-        let configured_app = self.build(app, options).await?;
+        let configured_app = self.build(app, common_options, options).await?;
         Ok(self.trigger.run(configured_app))
     }
 }
 
 pub trait RuntimeFactorsBuilder {
-    type Options;
+    type Options: clap::Args;
     type Factors: RuntimeFactors;
 
-    fn new() -> Self
-    where
-        Self: Sized;
-
     fn build(
-        self,
-        working_dir: PathBuf,
+        common_options: CommonTriggerOptions,
         options: Self::Options,
     ) -> anyhow::Result<(
         Self::Factors,

crates/trigger/src/cli/launch_metadata.rs

@@ -1,10 +1,11 @@
 use clap::CommandFactory;
 use serde::{Deserialize, Serialize};
-use spin_factors::RuntimeFactors;
 use std::ffi::OsString;
 
 use crate::{cli::FactorsTriggerCommand, Trigger};
 
+use super::RuntimeFactorsBuilder;
+
 /// Contains information about the trigger flags (and potentially
 /// in future configuration) that a consumer (such as `spin up`)
 /// can query using `--launch-metadata-only`.
@@ -28,8 +29,8 @@ struct LaunchFlag {
 }
 
 impl LaunchMetadata {
-    pub fn infer<T: Trigger<F>, F: RuntimeFactors>() -> Self {
-        let all_flags: Vec<_> = FactorsTriggerCommand::<T, F>::command()
+    pub fn infer<T: Trigger<B::Factors>, B: RuntimeFactorsBuilder>() -> Self {
+        let all_flags: Vec<_> = FactorsTriggerCommand::<T, B>::command()
             .get_arguments()
             .map(LaunchFlag::infer)
             .collect();

crates/trigger/src/factors.rs

@@ -1,6 +1,7 @@
 use std::path::PathBuf;
 
 use anyhow::Context as _;
+use spin_common::arg_parser::parse_kv;
 use spin_factor_key_value::KeyValueFactor;
 use spin_factor_llm::LlmFactor;
 use spin_factor_outbound_http::OutboundHttpFactor;
@@ -88,3 +89,22 @@ impl TryFrom<TomlRuntimeConfigSource<'_, '_>> for TriggerFactorsRuntimeConfig {
         Self::from_source(value)
     }
 }
+
+/// Options for building a [`TriggerFactors`].
+#[derive(Default, clap::Args)]
+pub struct TriggerAppOptions {
+    /// Set the static assets of the components in the temporary directory as writable.
+    #[clap(long = "allow-transient-write")]
+    pub allow_transient_write: bool,
+
+    /// Set a key/value pair (key=value) in the application's
+    /// default store. Any existing value will be overwritten.
+    /// Can be used multiple times.
+    #[clap(long = "key-value", parse(try_from_str = parse_kv))]
+    key_values: Vec<(String, String)>,
+
+    /// Run a SQLite statement such as a migration against the default database.
+    /// To run from a file, prefix the filename with @ e.g. spin up --sqlite @migration.sql
+    #[clap(long = "sqlite")]
+    sqlite_statements: Vec<String>,
+}

src/bin/spin.rs

@@ -19,8 +19,8 @@ use spin_cli::commands::{
 use spin_cli::{build_info::*, subprocess::ExitStatusError};
 use spin_runtime_config::{ResolvedRuntimeConfig, UserProvidedPath};
 use spin_trigger::cli::help::HelpArgsOnlyTrigger;
-use spin_trigger::cli::{FactorsTriggerCommand, RuntimeFactorsBuilder, TriggerAppOptions};
-use spin_trigger::{TriggerFactors, TriggerFactorsRuntimeConfig};
+use spin_trigger::cli::{CommonTriggerOptions, FactorsTriggerCommand, RuntimeFactorsBuilder};
+use spin_trigger::{TriggerAppOptions, TriggerFactors, TriggerFactorsRuntimeConfig};
 use spin_trigger_http::HttpTrigger;
 use spin_trigger_redis::RedisTrigger;
 
@@ -143,10 +143,10 @@ enum SpinApp {
 
 #[derive(Subcommand)]
 enum TriggerCommands {
-    Http(FactorsTriggerCommand<HttpTrigger, TriggerFactors>),
-    Redis(FactorsTriggerCommand<RedisTrigger, TriggerFactors>),
+    Http(FactorsTriggerCommand<HttpTrigger, Builder>),
+    Redis(FactorsTriggerCommand<RedisTrigger, Builder>),
     #[clap(name = spin_cli::HELP_ARGS_ONLY_TRIGGER_TYPE, hide = true)]
-    HelpArgsOnly(FactorsTriggerCommand<HelpArgsOnlyTrigger, TriggerFactors>),
+    HelpArgsOnly(FactorsTriggerCommand<HelpArgsOnlyTrigger, Builder>),
 }
 
 impl SpinApp {
@@ -161,9 +161,9 @@ impl SpinApp {
             Self::Login(cmd) => cmd.run(SpinApp::command()).await,
             Self::Registry(cmd) => cmd.run().await,
             Self::Build(cmd) => cmd.run().await,
-            Self::Trigger(TriggerCommands::Http(cmd)) => cmd.run::<Builder>().await,
-            Self::Trigger(TriggerCommands::Redis(cmd)) => cmd.run::<Builder>().await,
-            Self::Trigger(TriggerCommands::HelpArgsOnly(cmd)) => cmd.run::<Builder>().await,
+            Self::Trigger(TriggerCommands::Http(cmd)) => cmd.run().await,
+            Self::Trigger(TriggerCommands::Redis(cmd)) => cmd.run().await,
+            Self::Trigger(TriggerCommands::HelpArgsOnly(cmd)) => cmd.run().await,
             Self::Plugins(cmd) => cmd.run().await,
             Self::External(cmd) => execute_external_subcommand(cmd, app).await,
             Self::Watch(cmd) => cmd.run().await,
@@ -178,30 +178,22 @@ impl RuntimeFactorsBuilder for Builder {
     type Options = TriggerAppOptions;
     type Factors = TriggerFactors;
 
-    fn new() -> Self
-    where
-        Self: Sized,
-    {
-        Builder
-    }
-
     fn build(
-        self,
-        working_dir: PathBuf,
+        common_options: CommonTriggerOptions,
         options: Self::Options,
     ) -> anyhow::Result<(
         Self::Factors,
         <Self::Factors as spin_factors::RuntimeFactors>::RuntimeConfig,
     )> {
-        let runtime_config_path = options.runtime_config_file;
-        let local_app_dir = options.local_app_dir.map(PathBuf::from);
-        let state_dir = match options.state_dir {
+        let runtime_config_path = common_options.runtime_config_file;
+        let local_app_dir = common_options.local_app_dir.map(PathBuf::from);
+        let state_dir = match common_options.state_dir {
             // Make sure `--state-dir=""` unsets the state dir
             Some(s) if s.is_empty() => UserProvidedPath::Unset,
             Some(s) => UserProvidedPath::Provided(PathBuf::from(s)),
             None => UserProvidedPath::Default,
         };
-        let log_dir = match &options.log_dir {
+        let log_dir = match &common_options.log_dir {
             // Make sure `--log-dir=""` unsets the log dir
             Some(p) if p.as_os_str().is_empty() => UserProvidedPath::Unset,
             Some(p) => UserProvidedPath::Provided(p.clone()),
@@ -225,7 +217,7 @@ impl RuntimeFactorsBuilder for Builder {
         let log_dir = runtime_config.log_dir();
         let factors = TriggerFactors::new(
             runtime_config.state_dir(),
-            working_dir,
+            common_options.working_dir,
             options.allow_transient_write,
             runtime_config.key_value_resolver,
             runtime_config.sqlite_resolver,