-
Notifications
You must be signed in to change notification settings - Fork 77
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
Update shim dependencies #95
Conversation
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
@@ -21,6 +21,7 @@ use std::os::unix::io::{AsRawFd, FromRawFd}; | |||
use std::path::Path; | |||
use std::sync::mpsc::{sync_channel, Receiver, SyncSender}; | |||
|
|||
use containerd_shim::protos::protobuf::EnumOrUnknown; |
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.
should we make a rule of referencing packages order, like std/3rd/self
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've fixed imports in 702d021
Can you elaborate on "make a rule"? I could not find the way to enforce imports order on CI :(
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.
Same with golang projects, I need to seperate these packages into three parts(std/3rd/self) by myself, the order of each part will be updated automatically by IDE or rust plugins. Thus, is it necessary to make demarcation? I also see many other repos they don't care about that.
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.
If it's troublesome, just let it go as it doesn't impact compile.
@@ -13,3 +13,6 @@ pub mod shim_ttrpc_async; | |||
pub(crate) mod empty; | |||
pub(crate) mod mount; | |||
pub(crate) mod task; | |||
|
|||
mod fieldpath; | |||
mod gogo; |
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.
public or not?
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 plan to remove the generated code from the repo, so I don't have strong preference here.
crates/shim/src/cgroup.rs
Outdated
let mut cpu_stat = CPUStat::new(); | ||
cpu_stat.set_usage(cpu_usage); | ||
metrics.set_cpu(cpu_stat); | ||
cpu_stat.usage = Some(cpu_usage).into(); |
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.
Does the new ttrpc not provide setter functions? I think Some(xxx).into
is a little complex by writing.
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.
Protobuf 3 has removed setters/getters for generated structs.
usage
has MessageField type, which is a wrapper for Option<Box<T>>
(replacement for SingularPtrField
used in Protobuf 2). I found Some().into()
to be the shortest syntax possible here.
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 also possible to use MessageField::some()
, but it looks weird in the context of Rust with std Option
type. So I'd prefer to use Some
here.
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.
that's OK
Signed-off-by: Maksym Pavlenko <pavlenko.maksym@gmail.com>
So I just found |
ping @Burning1020 |
Lgtm |
Bump containerd-shim-protos from 0.2.0 to 0.3.0 to include changes made in containerd#95. Because the update in containerd#95 is not compatible we need a major version update. Signed-off-by: Tim Zhang <tim@hyper.sh>
This PR updates shim dependencies (updates ttrpc to latest versions).
Since TTRPC is now using latest protobuf 3.1, there is some migration work.
Close #93 #92 #91 #82