-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: container-type level version compatibility check #190
Conversation
Signed-off-by: Keran Yang <[email protected]>
Signed-off-by: Keran Yang <[email protected]>
Signed-off-by: Keran Yang <[email protected]>
Signed-off-by: Keran Yang <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #190 +/- ##
==========================================
- Coverage 94.53% 94.10% -0.43%
==========================================
Files 56 56
Lines 2213 2241 +28
Branches 131 131
==========================================
+ Hits 2092 2109 +17
- Misses 90 101 +11
Partials 31 31 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Keran Yang <[email protected]>
Signed-off-by: Keran Yang <[email protected]>
serv_info = ServerInfo.get_default_server_info() | ||
serv_info.minimum_numaflow_version = MINIMUM_NUMAFLOW_VERSION[ | ||
ContainerType.Sourcetransformer | ||
] |
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.
not sure why tests are not covering it; it should be covered since there are no branches and the func is being called. interestingly it is happening everywhere.
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.
yes, I am also curious about it. Some of the servers actually got covered, e.g., sink server. @kohlisid do you know why?
@KeranYang I see this got merged but to answer your question, in our code the server start utility would add the default server info automatically,
Only if a deviation from default was required we could pass a custom info object cc @vigith |
I am thinking whether we use the same pattern we used in Rust SDK. |
We could have passed container type as parameter that can give the default with the requested one. |
thank you @kohlisid ! |
No description provided.