-
Notifications
You must be signed in to change notification settings - Fork 247
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
fix: move component filtering methods to crates #2892
base: main
Are you sure you want to change the base?
fix: move component filtering methods to crates #2892
Conversation
Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, couple of questions/checks but nothing important!
@@ -27,6 +29,8 @@ 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"); | |||
|
|||
type ValidatorFn = dyn Fn(&App, &[String]) -> anyhow::Result<()>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is used in a public interface, I wonder if it needs a pub
or a doc comment or something? I'm not sure what the expectation is with type aliases (and clearly the compiler doesn't mind).
/// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
|
||
[dev-dependencies] | ||
toml = { workspace = true } | ||
tempfile = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used?
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_components
to not be specific to CLI input.