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

factors: key value tests, doc comments, and Azure factor #2666

Merged
Merged
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
77 changes: 77 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 19 additions & 0 deletions crates/factor-key-value-azure/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
[package]
name = "spin-factor-key-value-azure"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that naming is a bit up in the air especially since we have a bunch of crates that will eventually go away, but I wonder if we already have an idea if this naming convention is worth keeping.

Imaging a world where the transition to factors is complete, I would personally like to see the following naming conventions:

  • For the factor themselves: factor-$import (e.g., factor-llm or factor-key-value). Even in a world where factors are not new, I think naming the crates with factor makes sense so that we can quickly see where all of the factors are defined. This is always a reason to start with factor_ so that all of the factors are bundled together.
    • An issue with this naming however, is that it's not clear in a global namespace like on crates.io what the crated does...
  • For Spin CLI specific implementations that wrap the factors: $import-$impl_specific_description (e.g., key-value-azure or sqlite-local-database). Keeping the kind of import first, means all related implementations will be bundled (e.g., everything key-value related will be bundled).
    • The thing I'm unsure about is how to address the need to specify that this is what is used for Spin CLI. The issue is that "spin" is used for much more than the Spin CLI. I can also imagine these being used by some other runtime (e.g., perhaps SpinKube will use some of these). I'm not sure if there's a terse way to label these crates to concisely show where they're used.
    • Of course, the issue listed above for the factor crates is also an issue here: publishing these on crates.io is difficult where they need to be unique and reasonably clear when viewed outside the context of this repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An issue with this naming however, is that it's not clear in a global namespace like on crates.io what the crated does...

Right now, it seems like the majority of our crates are spin prefixed for this discoverability reason

The issue is that "spin" is used for much more than the Spin CLI. I can also imagine these being used by some other runtime (e.g., perhaps SpinKube will use some of these). I'm not sure if there's a terse way to label these crates to concisely show where they're used.

The shim does and will likely continue to use these KV factors that the spin CLI uses. I wouldn't put Spin CLI ownership on them. Could it be assumed that if the naming scheme is spin-factor-$import then it is a factor and if it has a suffix with $impl_specific_description (spin-factor-$import-$impl_specific_description) then it is a specific implementation that a runtime could use. It just happens to be that the Spin CLI runtime authored the implementation

version.workspace = true
authors.workspace = true
edition.workspace = true
license.workspace = true
homepage.workspace = true
repository.workspace = true
rust-version.workspace = true

[dependencies]
anyhow = "1.0"
serde = { version = "1.0", features = ["rc"] }
spin-factor-key-value = { path = "../factor-key-value" }
# TODO: merge with this crate
spin-key-value-azure = { path = "../key-value-azure" }

[lints]
workspace = true
40 changes: 40 additions & 0 deletions crates/factor-key-value-azure/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
use serde::Deserialize;
use spin_factor_key_value::MakeKeyValueStore;
use spin_key_value_azure::KeyValueAzureCosmos;

/// A key-value store that uses Azure Cosmos as the backend.
pub struct AzureKeyValueStore;

/// Runtime configuration for the Azure Cosmos key-value store.
#[derive(Deserialize)]
pub struct AzureCosmosKeyValueRuntimeConfig {
/// The authorization token for the Azure Cosmos DB account.
key: String,
/// The Azure Cosmos DB account name.
account: String,
/// The Azure Cosmos DB database.
database: String,
/// The Azure Cosmos DB container where data is stored.
/// The CosmosDB container must be created with the default partition key, /id
container: String,
}

impl MakeKeyValueStore for AzureKeyValueStore {
const RUNTIME_CONFIG_TYPE: &'static str = "azure_cosmos";

type RuntimeConfig = AzureCosmosKeyValueRuntimeConfig;

type StoreManager = KeyValueAzureCosmos;

fn make_store(
&self,
runtime_config: Self::RuntimeConfig,
) -> anyhow::Result<Self::StoreManager> {
KeyValueAzureCosmos::new(
runtime_config.key,
runtime_config.account,
runtime_config.database,
runtime_config.container,
)
}
}
4 changes: 4 additions & 0 deletions crates/factor-key-value-redis/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
use serde::Deserialize;
use spin_factor_key_value::MakeKeyValueStore;
use spin_key_value_redis::KeyValueRedis;

/// A key-value store that uses Redis as the backend.
pub struct RedisKeyValueStore;

/// Runtime configuration for the Redis key-value store.
#[derive(Deserialize)]
pub struct RedisKeyValueRuntimeConfig {
/// The URL of the Redis server.
url: String,
}

Expand Down
22 changes: 14 additions & 8 deletions crates/factor-key-value-spin/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,30 +8,36 @@ use serde::{Deserialize, Serialize};
use spin_factor_key_value::MakeKeyValueStore;
use spin_key_value_sqlite::{DatabaseLocation, KeyValueSqlite};

/// A key-value store that uses SQLite as the backend.
pub struct SpinKeyValueStore {
/// The base path or directory for the SQLite database file.
base_path: PathBuf,
}

impl SpinKeyValueStore {
pub fn new(base_path: Option<PathBuf>) -> anyhow::Result<Self> {
let base_path = match base_path {
Some(path) => path,
None => std::env::current_dir().context("failed to get current directory")?,
};
Ok(Self { base_path })
/// Create a new SpinKeyValueStore with the given base path.
pub fn new(base_path: PathBuf) -> Self {
Self { base_path }
}
}

/// Runtime configuration for the SQLite key-value store.
#[derive(Deserialize, Serialize)]
pub struct SpinKeyValueRuntimeConfig {
/// The path to the SQLite database file.
path: Option<PathBuf>,
}

impl SpinKeyValueRuntimeConfig {
/// The default filename for the SQLite database.
const DEFAULT_SPIN_STORE_FILENAME: &'static str = "sqlite_key_value.db";

pub fn default(state_dir: Option<PathBuf>) -> Self {
let path = state_dir.map(|dir| dir.join(Self::DEFAULT_SPIN_STORE_FILENAME));
/// Create a new runtime configuration with the given state directory.
///
/// If the database directory is None, the database is in-memory.
/// If the database directory is Some, the database is stored in a file in the state directory.
pub fn default(default_database_dir: Option<PathBuf>) -> Self {
let path = default_database_dir.map(|dir| dir.join(Self::DEFAULT_SPIN_STORE_FILENAME));
Self { path }
}
}
Expand Down
8 changes: 8 additions & 0 deletions crates/factor-key-value/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,13 @@ spin-key-value = { path = "../key-value" }
spin-world = { path = "../world" }
toml = "0.8"

[dev-dependencies]
spin-factors-test = { path = "../factors-test" }
tokio = { version = "1", features = ["macros", "rt"] }
spin-factor-key-value-spin = { path = "../factor-key-value-spin" }
spin-factor-key-value-redis = { path = "../factor-key-value-redis" }
tempdir = "0.3"


[lints]
workspace = true
29 changes: 26 additions & 3 deletions crates/factor-key-value/src/delegating_resolver.rs
Original file line number Diff line number Diff line change
@@ -1,26 +1,34 @@
use crate::runtime_config::RuntimeConfigResolver;
use crate::store::{store_from_toml_fn, MakeKeyValueStore, StoreFromToml};
use serde::Deserialize;
use serde::{Deserialize, Serialize};
use spin_key_value::StoreManager;
use std::{collections::HashMap, sync::Arc};

/// A RuntimeConfigResolver that delegates to the appropriate key-value store
/// for a given label.
///
/// The store types are registered with the resolver using `add_store_type`. The
/// default store for a label is registered using `add_default_store`.
#[derive(Default)]
pub struct DelegatingRuntimeConfigResolver {
/// A map of store types to a function that returns the appropriate store
/// manager from runtime config TOML.
store_types: HashMap<&'static str, StoreFromToml>,
/// A map of default store configurations for a label.
defaults: HashMap<&'static str, StoreConfig>,
}

impl DelegatingRuntimeConfigResolver {
/// Create a new DelegatingRuntimeConfigResolver.
pub fn new() -> Self {
Self::default()
}

pub fn add_default_store(&mut self, label: &'static str, config: StoreConfig) {
self.defaults.insert(label, config);
}
}

impl DelegatingRuntimeConfigResolver {
/// Adds a store type to the resolver.
pub fn add_store_type<T: MakeKeyValueStore>(&mut self, store_type: T) -> anyhow::Result<()> {
if self
.store_types
Expand Down Expand Up @@ -48,6 +56,9 @@ impl RuntimeConfigResolver for DelegatingRuntimeConfigResolver {
store_from_toml(config.config)
}

/// Get the default store manager for the given label.
///
/// Returns None if no default store is registered for the label.
fn default_store(&self, label: &str) -> Option<Arc<dyn StoreManager>> {
let config = self.defaults.get(label)?;
self.get_store(config.clone()).ok()
Expand All @@ -61,3 +72,15 @@ pub struct StoreConfig {
#[serde(flatten)]
pub config: toml::Table,
}

impl StoreConfig {
pub fn new<T>(type_: String, config: T) -> anyhow::Result<Self>
where
T: Serialize,
{
Ok(Self {
type_,
config: toml::value::Table::try_from(config)?,
})
}
}
Loading
Loading