-
Notifications
You must be signed in to change notification settings - Fork 249
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
Refactor node builders [ECR-4298] #1800
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1800 +/- ##
=========================================
- Coverage 94.8% 94.8% -0.01%
=========================================
Files 269 269
Lines 42044 42044
=========================================
- Hits 39861 39859 -2
- Misses 2183 2185 +2
Continue to review full report at Codecov.
|
|
||
/// Marker type for artifact deployment without migration support. | ||
#[derive(Debug)] | ||
pub struct Simple(()); |
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.
Why not just Service
?
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.
I think Simple
/ Migrating
dichotomy provides a better description than Service
/ Migrating
. (For one, "simple" and "migrating" are both adjectives 🙃)
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.
Oh, that is my mistake, I meant that we can just write struct Simple
instead of struct Simple(())
.
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.
I wanted Simple
/ Migrating
to be marker types (i.e., not instantiable).
runtimes/rust/src/spec.rs
Outdated
#[derive(Debug)] | ||
pub struct Migrating(()); | ||
|
||
/// Deploy specification for a Rust artifact. The spec can include 0 or more instantiated services. |
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.
Imo zero instead of 0 is more readable.
Overview
This PR refactors node builders in the testkit and the CLI crate, extracting reusable parts into a separate extendable interface.
See: https://jira.bf.local/browse/ECR-4298