fix: move component filtering methods to crates #2892
Conversation
| /// component is configured to to chain to another component that is not | ||
| /// retained. All wildcard service chaining is disallowed and all templated URLs | ||
| /// are ignored. | ||
| pub fn validate_service_chaining_for_components( |
There was a problem hiding this comment.
It might be desirable to give this a more specific name but the doc comment covers it well and the only names I can think of are a bit verbose (e.g. validate_retained_components_include_service_chaining_dependencies!). Not a blocker, just if inspiration strikes!
There was a problem hiding this comment.
Even though this is used by Spin when retaining only a set of components, technically this could be used elsewhere to establish that a set of components have satisfied service chaining. Maybe a runtime implementor would use this for app splitting ... but i like the vague name because technically the functionality could be used in another objective
Signed-off-by: Kate Goldenring <[email protected]>
kate-goldenring
force-pushed
the
component-filtering-move
branch
from
a071b6d
to
b01be64
Compare
crates/app/src/lib.rs
Outdated
| @@ -27,6 +29,10 @@ pub const APP_DESCRIPTION_KEY: MetadataKey = MetadataKey::new("description"); | |||
| /// MetadataKey for extracting the OCI image digest. | |||
| pub const OCI_IMAGE_DIGEST_KEY: MetadataKey = MetadataKey::new("oci_image_digest"); | |||
|
|
|||
| /// Validation function type for ensuring that applications meet requirements | |||
| /// even with components filtered out. | |||
| pub type ValidatorFn = dyn Fn(&App, &[String]) -> anyhow::Result<()>; | |||
There was a problem hiding this comment.
Nit: I typically find using a trait for stuff like this a bit more self documenting . That way you can give the function params names. For example:
trait ApplicationValidator {
fn validate(&self, app: &App, retained_component_ids: &[String]);
}
There was a problem hiding this comment.
This is a great approach. It does make the implementation for the runtime a little clunky though, having to define a trait and then add the validating function to it. I like the simplicity of passing in just a function, though we do lose the better documentation of params
crates/app/src/lib.rs
Outdated
| /// Introspects the LockedApp to find and selectively retain the triggers that correspond to those components | ||
| fn retain_components( | ||
| mut self, | ||
| retained_components: &[String], |
There was a problem hiding this comment.
Nit: I think it might be nicer if retained_components here (and in other functions) was an impl Iterator<Item = &str> so that we could avoid a whole bunch of to_string() calls.
There was a problem hiding this comment.
I am not able to find a way to satisfy lifetimes with this and expressing the function as a type as impl aliases are not allowed in type definitions. But i did change the parameters to take in &[&str] instead.
| } | ||
| } | ||
|
|
||
| pub async fn build_locked_app(manifest: &toml::Table) -> anyhow::Result<LockedApp> { |
There was a problem hiding this comment.
Should this be a TryFrom<toml::Toml> for LockedApp impl?
There was a problem hiding this comment.
I dont see us using it anywhere outside of tests so it may make sense to avoid moving up to the crate
Signed-off-by: Kate Goldenring <[email protected]>
Moves all the component filtering helpers to crates that are accessible to other runtime implementors to prevent code duplication. Also, enables runtimes to pass in extra validating functions.
Thank you @itowlson for pairing on this!
Most of the code is the same. I did change the error message on
validate_service_chaining_for_componentsto not be specific to CLI input.