Spin deps CLI - #3660
Conversation
Signed-off-by: Brian Hardock <brian.hardock@fermyon.com>
Signed-off-by: Brian Hardock <brian.hardock@fermyon.com>
Signed-off-by: Brian Hardock <brian.hardock@fermyon.com>
itowlson
left a comment
itowlson
left a comment
There was a problem hiding this comment.
This looks good! I struggled with a few things I'm afraid, and I have some naming nits. My main feeling is that I would really like to see the main flows as something like:
fn add_component_dependency() {
let component_id = select_component(); // }
let selected_interface = select_interface(); // } as you already have it
let inheritance = select_inheritance(); // }
write_dependency_to_manifest();
regenerate_dependencies_wit();
print_report();
}
instead if inlining the last half, which for me meant I lost track of what is really a very simple linear flow.
But all that aside the bones seem really solid and it is mostly stylistic confusion on my part. Great to see this under sail!
| .context("Failed to access dependencies table")?; | ||
|
|
||
| let dep_key = dep_name.to_string(); | ||
| if deps_table.contains_key(&dep_key) { |
There was a problem hiding this comment.
This feels like a thing we could check earlier, before tormenting them with decisions about capabilities that turn out to be completely nugatory. (We could even not offer keys that already exist.)
| inherit_configuration, | ||
| } => { | ||
| let mut table = toml_edit::InlineTable::new(); | ||
| table.insert( |
There was a problem hiding this comment.
Disappointing that we can't use toml serialisation and slurp that into a toml_edit::Item.
| package, version, .. | ||
| } => match package { | ||
| Some(p) => format!("{p}@{version}"), | ||
| None => version.clone(), |
There was a problem hiding this comment.
Not sure where this is used but the comment implies it's human-facing - if so, I'm somewhat worried that a bare version may not be very informative unless it has the package name nearby. But like I say I am missing context on this
| } | ||
|
|
||
| /// Serialize the resolved source as a middleware entry inline table. | ||
| fn serialize_trigger_dependency( |
There was a problem hiding this comment.
This seems very reminiscent of serialise_component_dependency. Even if we can't consolidate them, I'd consider locating them next to each other in the file (or even in a sub-module) so that someone maintaining them is more likely to see them both at once.
| } | ||
|
|
||
| /// Print guidance about the capabilities the middleware needs. | ||
| fn print_middleware_capability_guidance( |
There was a problem hiding this comment.
This seems very similar to the other "print capability guidance" function except for the word "middleware" in the messages. Could we have a single function that takes a parameter? Do we even need to say "middleware" instead of "dependency" given that middleware is enough of a dependency to live under the dependencies command?
Signed-off-by: Brian Hardock <brian.hardock@fermyon.com>
Signed-off-by: Brian Hardock <brian.hardock@fermyon.com>
2 participants
Implementation of SIP 024.
The first 2 commits here are two separable / independently reviewable chunks of work to (1) detect capability sets and (2) detect middleware.
The actual CLI experience (3rd commit) is still WIP (but feedback welcome regardless).