fix: move component filtering methods to crates#2892
fix: move component filtering methods to crates#2892itowlson merged 2 commits intospinframework:mainfrom
Conversation
itowlson
left a comment
itowlson
left a comment
There was a problem hiding this comment.
LGTM, couple of questions/checks but nothing important!
| /// 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 <kate.goldenring@fermyon.com>
kate-goldenring
force-pushed
the
component-filtering-move
branch
from
a071b6d to
b01be64
Compare
rylev
left a comment
rylev
left a comment
There was a problem hiding this comment.
Looks good - I only have a few nits and suggestions, but feel free to ignore them.
crates/app/src/lib.rs
Outdated
|
|
||
| /// 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 <kate.goldenring@fermyon.com>
3 participants
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.