-
Notifications
You must be signed in to change notification settings - Fork 30
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
Generate CoreDB Spec from Stack Spec #772
Conversation
metadata: serde_json::json!({ | ||
"name": resource_name, | ||
}), | ||
spec: spec.clone(), |
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's a short-lived CLI tool, so it probably doesn't matter, but I'm trying to practice with the borrow checker and think it would be pretty easy to just pass ownership of spec
to this function at the point of calling it, right? It's never used again in the calling function.
That said, it won't affect correctness or performance to use clone
, I'm just reading to learn Rust better.
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.
Correct.
@@ -175,6 +214,20 @@ fn default_storage() -> String { | |||
"10Gi".to_owned() | |||
} | |||
|
|||
pub fn merge_options<T>(opt1: Option<Vec<T>>, opt2: Option<Vec<T>>) -> Option<Vec<T>> |
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.
Unless I missed something, this isn't called in the changes in this review, right? What's it for?
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.
Looks good to me. Left two non-blocking comments as questions for myself, but they're not related to approval.
Quick way to generate a CoreDB Spec from a Stack spec. This wont work for all cases, and definitely not for production usage but will be handy for local development.