Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trigger: Decouple Trigger from clap #2864

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions crates/trigger-http/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ pub(crate) type TriggerInstanceBuilder<'a, F> =
spin_trigger::TriggerInstanceBuilder<'a, HttpTrigger, F>;

#[derive(Args)]
pub struct CliArgs {
pub struct GlobalConfig {
/// IP address and port to listen on
#[clap(long = "listen", env = "SPIN_HTTP_LISTEN_ADDR", default_value = "127.0.0.1:3000", value_parser = parse_listen_addr)]
pub address: SocketAddr,
Expand All @@ -52,7 +52,7 @@ pub struct CliArgs {
pub tls_key: Option<PathBuf>,
}

impl CliArgs {
impl GlobalConfig {
fn into_tls_config(self) -> Option<TlsConfig> {
match (self.tls_cert, self.tls_key) {
(Some(cert_path), Some(key_path)) => Some(TlsConfig {
Expand All @@ -78,11 +78,11 @@ pub struct HttpTrigger {
impl<F: RuntimeFactors> Trigger<F> for HttpTrigger {
const TYPE: &'static str = "http";

type CliArgs = CliArgs;
type GlobalConfig = GlobalConfig;
type InstanceState = ();

fn new(cli_args: Self::CliArgs, app: &spin_app::App) -> anyhow::Result<Self> {
Self::new(app, cli_args.address, cli_args.into_tls_config())
fn new(cfg: Self::GlobalConfig, app: &spin_app::App) -> anyhow::Result<Self> {
Self::new(app, cfg.address, cfg.into_tls_config())
}

async fn run(self, trigger_app: TriggerApp<F>) -> anyhow::Result<()> {
Expand Down
6 changes: 3 additions & 3 deletions crates/trigger-redis/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use redis::{Client, Msg};
use serde::Deserialize;
use spin_factor_variables::VariablesFactor;
use spin_factors::RuntimeFactors;
use spin_trigger::{cli::NoCliArgs, App, Trigger, TriggerApp};
use spin_trigger::{cli::NoGlobalConfig, App, Trigger, TriggerApp};
use spin_world::exports::fermyon::spin::inbound_redis;
use tracing::{instrument, Level};

Expand Down Expand Up @@ -34,11 +34,11 @@ struct TriggerConfig {
impl<F: RuntimeFactors> Trigger<F> for RedisTrigger {
const TYPE: &'static str = "redis";

type CliArgs = NoCliArgs;
type GlobalConfig = NoGlobalConfig;

type InstanceState = ();

fn new(_cli_args: Self::CliArgs, _app: &App) -> anyhow::Result<Self> {
fn new(_cfg: Self::GlobalConfig, _app: &App) -> anyhow::Result<Self> {
Ok(Self)
}

Expand Down
28 changes: 19 additions & 9 deletions crates/trigger/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,12 @@ pub const SPIN_WORKING_DIR: &str = "SPIN_WORKING_DIR";
usage = "spin [COMMAND] [OPTIONS]",
next_help_heading = help_heading::<T, B::Factors>()
)]
pub struct FactorsTriggerCommand<T: Trigger<B::Factors>, B: RuntimeFactorsBuilder> {
pub struct FactorsTriggerCommand<T, B>
where
T: Trigger<B::Factors>,
T::GlobalConfig: Args,
B: RuntimeFactorsBuilder,
{
/// Log directory for the stdout and stderr of components. Setting to
/// the empty string disables logging to disk.
#[clap(
Expand Down Expand Up @@ -110,7 +115,7 @@ pub struct FactorsTriggerCommand<T: Trigger<B::Factors>, B: RuntimeFactorsBuilde
pub state_dir: Option<String>,

#[clap(flatten)]
pub trigger_args: T::CliArgs,
pub trigger_args: T::GlobalConfig,

#[clap(flatten)]
pub builder_args: B::CliArgs,
Expand Down Expand Up @@ -139,12 +144,17 @@ pub struct FactorsConfig {
pub log_dir: UserProvidedPath,
}

/// An empty implementation of clap::Args to be used as TriggerExecutor::RunConfig
/// for executors that do not need additional CLI args.
/// This type may be used as the [`Trigger::GlobalConfig`] for triggers with no
/// CLI args.
#[derive(Args)]
pub struct NoCliArgs;

impl<T: Trigger<B::Factors>, B: RuntimeFactorsBuilder> FactorsTriggerCommand<T, B> {
pub struct NoGlobalConfig;

impl<T, B> FactorsTriggerCommand<T, B>
where
T: Trigger<B::Factors>,
T::GlobalConfig: Args,
B: RuntimeFactorsBuilder,
{
/// Create a new TriggerExecutorBuilder from this TriggerExecutorCommand.
pub async fn run(self) -> Result<()> {
// Handle --help-args-only
Expand Down Expand Up @@ -383,10 +393,10 @@ pub mod help {

impl<F: RuntimeFactors> Trigger<F> for HelpArgsOnlyTrigger {
const TYPE: &'static str = "help-args-only";
type CliArgs = NoCliArgs;
type GlobalConfig = NoGlobalConfig;
type InstanceState = ();

fn new(_cli_args: Self::CliArgs, _app: &App) -> anyhow::Result<Self> {
fn new(_cfg: Self::GlobalConfig, _app: &App) -> anyhow::Result<Self> {
Ok(Self)
}

Expand Down
9 changes: 7 additions & 2 deletions crates/trigger/src/cli/launch_metadata.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use clap::CommandFactory;
use clap::{Args, CommandFactory};
use serde::{Deserialize, Serialize};
use std::ffi::OsString;

Expand Down Expand Up @@ -29,7 +29,12 @@ struct LaunchFlag {
}

impl LaunchMetadata {
pub fn infer<T: Trigger<B::Factors>, B: RuntimeFactorsBuilder>() -> Self {
pub fn infer<T, B>() -> Self
where
T: Trigger<B::Factors>,
T::GlobalConfig: Args,
B: RuntimeFactorsBuilder,
{
let all_flags: Vec<_> = FactorsTriggerCommand::<T, B>::command()
.get_arguments()
.map(LaunchFlag::infer)
Expand Down
16 changes: 8 additions & 8 deletions crates/trigger/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ pub mod loader;

use std::future::Future;

use clap::Args;
use spin_core::Linker;
use spin_factors::RuntimeFactors;
pub use spin_core::Linker;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised by this. I looked at a few of the external triggers, and they don't use Linker. Is this really necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its used in the (admittedly optional) Trigger::add_to_linker signature.

pub use spin_factors::RuntimeFactors;
use spin_factors_executor::{FactorsExecutorApp, FactorsInstanceBuilder};

pub use anyhow;
pub use spin_app::App;

/// Type alias for a [`spin_factors_executor::FactorsExecutorApp`] specialized to a [`Trigger`].
Expand All @@ -21,7 +21,7 @@ pub type TriggerInstanceBuilder<'a, T, F> =
pub type Store<T, F> = spin_core::Store<TriggerInstanceState<T, F>>;

/// Type alias for [`spin_factors_executor::InstanceState`] specialized to a [`Trigger`].
type TriggerInstanceState<T, F> = spin_factors_executor::InstanceState<
pub type TriggerInstanceState<T, F> = spin_factors_executor::InstanceState<
<F as RuntimeFactors>::InstanceState,
<T as Trigger<F>>::InstanceState,
>;
Expand All @@ -31,14 +31,14 @@ pub trait Trigger<F: RuntimeFactors>: Sized + Send {
/// A unique identifier for this trigger.
const TYPE: &'static str;

/// The specific CLI arguments for this trigger.
type CliArgs: Args;
/// Global configuration used to construct this trigger.
type GlobalConfig;

/// The instance state for this trigger.
/// Extra state attached to each instance store for this trigger.
type InstanceState: Send + 'static;

/// Constructs a new trigger.
fn new(cli_args: Self::CliArgs, app: &App) -> anyhow::Result<Self>;
fn new(cfg: Self::GlobalConfig, app: &App) -> anyhow::Result<Self>;

/// Update the [`spin_core::Config`] for this trigger.
///
Expand Down
8 changes: 4 additions & 4 deletions examples/spin-timer/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ wasmtime::component::bindgen!({
});

#[derive(Args)]
pub struct CliArgs {
pub struct GlobalConfig {
/// If true, run each component once and exit
#[clap(long)]
pub test: bool,
Expand Down Expand Up @@ -49,11 +49,11 @@ pub struct TimerTriggerConfig {
impl<F: RuntimeFactors> Trigger<F> for TimerTrigger {
const TYPE: &'static str = "timer";

type CliArgs = CliArgs;
type GlobalConfig = GlobalConfig;

type InstanceState = ();

fn new(cli_args: Self::CliArgs, app: &App) -> anyhow::Result<Self> {
fn new(cfg: Self::GlobalConfig, app: &App) -> anyhow::Result<Self> {
let trigger_type = <Self as Trigger<F>>::TYPE;
let metadata = app
.get_trigger_metadata::<TriggerMetadata>(trigger_type)?
Expand All @@ -67,7 +67,7 @@ impl<F: RuntimeFactors> Trigger<F> for TimerTrigger {
.collect();

Ok(Self {
test: cli_args.test,
test: cfg.test,
speedup,
component_timings,
})
Expand Down
2 changes: 1 addition & 1 deletion tests/testing-framework/src/runtimes/in_process_spin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use std::sync::Arc;

use anyhow::Context as _;
use spin_runtime_factors::{FactorsBuilder, TriggerAppArgs, TriggerFactors};
use spin_runtime_factors::{FactorsBuilder, TriggerFactors, TriggerAppArgs};
use spin_trigger::{cli::TriggerAppBuilder, loader::ComponentLoader};
use spin_trigger_http::{HttpServer, HttpTrigger};
use test_environment::{
Expand Down
Loading