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

Conversation

kate-goldenring
Copy link
Contributor

Adds inline docs, tests, and the final KV Azure factor

@kate-goldenring
Copy link
Contributor Author

kate-goldenring commented Jul 19, 2024

@rylev I can rebase and add in the generic substitution for the Azure factor too once this is in: #2665

Copy link
Collaborator

@rylev rylev left a comment

Choose a reason for hiding this comment

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

Looks like a clear improvement. I have some points for discussion, but I don't think we really need to block on merging this.

@@ -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

crates/factor-key-value-spin/src/lib.rs Outdated Show resolved Hide resolved
crates/factor-key-value-spin/src/lib.rs Outdated Show resolved Hide resolved
crates/factor-key-value-spin/src/lib.rs Outdated Show resolved Hide resolved
crates/factor-key-value/src/lib.rs Outdated Show resolved Hide resolved
crates/factor-key-value/tests/test.rs Outdated Show resolved Hide resolved
crates/factor-key-value/tests/test.rs Outdated Show resolved Hide resolved
Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
…tactical nits

Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
@kate-goldenring kate-goldenring merged commit 2d709df into fermyon:factors Jul 25, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants