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
Add rootless for spec #49
Conversation
As discussed in containers/youki#77 |
@saschagrunert my changes failed for Those attributes in Linux is private ... |
This is probably caused by the fact that the field of the structure is private when there is a feature builder pattern. So you need to distinguish between the case where the feature builder is present and the case where it is not. Lines 1 to 41 in 26101ac
|
Emm, this will make code duplicated. I haven't got the benefits of getset. |
Fixed it with some time. I'm not a fan of OOP, and don't think it's more convenient with a builder. And in the future, we will only keep the feature = "builder", right? |
Codecov Report
@@ Coverage Diff @@
## main #49 +/- ##
==========================================
+ Coverage 26.93% 27.26% +0.33%
==========================================
Files 19 19
Lines 1344 1401 +57
Branches 694 737 +43
==========================================
+ Hits 362 382 +20
- Misses 374 377 +3
- Partials 608 642 +34 |
A practical benefit of using the builder pattern is that end-users directly know when the struct is in "locked-up" in read-only mode. When creating the spec, we only pass around the builder. At the moment we call Sure, we could also use
I don't know. I made it optional for folks to select on their personal preference and make the migration in youki easier. |
I also got quite confused by this "fields are private when using the
I don't understand your statement. In Rust variables aren't mutable by default, so if I have e.g. |
In the case of the builder pattern, the structure for building is different from the structure that is actually built by the builder pattern, so by setting the field of the structure to be built to private, you can't change it after the last I thought that was what he meant. I'm sorry if I'm wrong. |
Anyway, the |
Right now, but the intention of this crate was to use it from other consumers as well.
I'm open for that, do we wanna start a vote in a dedicated issue? |
|
Personally I like the builder pattern, but it seems to be confusing for people. Maybe this can be solved with documentation? Right now this feature is not really explained. Adding a description of the feature and the rationale to the readme could help in that regard. |
@chenyukang You could pass it in as as argument for set_for_rootless. I would prefer it that way actually because currently if you calls this as root, container root will be mapped to host root which is usually not what you want. |
@@ -31,6 +31,7 @@ serde_json = "1.0.66" | |||
quickcheck = { version = "1.0.3", optional = true } | |||
derive_builder = { version = "0.10.2", optional = true } | |||
getset = { version = "0.1.1", optional = true } | |||
path-clean = "0.1.0" |
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.
@saschagrunert What do you think about the introduction of this crate?
https://github.com/danreeves/path-clean
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.
Would it make sense to make it optional and make rootless a feature?
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.
Would it make sense to make it optional and make rootless a feature?
I think it is OK.
Do we have to implement the set_for_rootless in this crate? To me, we should limit what this crate does so it can be shared with more repos. Can we in the main Youki repo create a spec that's rootless? Honestly, I am not sure how I feel about the builder pattern, but maintaining two version of the same code is not sustainable in my opinion. |
I also have some considerations when implementing it. I thought put it in this repo because it seems more natural, out of oci-spec, we get a json and don't know much details about it. And we currently also have I agree with two versions of the same code is not sustainable. |
I think it would be easier to discuss the builder pattern here in the future, so what do you think? We are definitely looking for great feedback. |
@yihuaf @chenyukang |
@chenyukang |
I haven't tried it. Youki currently hasn't synced with the head of
oci-spec-rs.
We'd better update it to the latest and then have a try.
I am not sure whether all the data structures internals can be accessed
outside of oci-spec-rs.
…On Fri, Sep 3, 2021 at 9:47 AM yihuaf ***@***.***> wrote:
@chenyukang <https://github.com/chenyukang> canonicalize_rootfs should be
outside of this repo and inside the main repo as well. It was historical
since we didn't take care of this when breaking this repo off from the
Youki main repo. Depending on what we decide on #60
<#60>, I will probably
move that out of this repo as well. Is there any difficulties moving
set_for_rootless into the main Youki repo? Maybe this is the purist in me
speaking, but I think we should do that if we can.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#49 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABYJ5W76DIIVEJPC4KM4I3UAASMJANCNFSM5C5ZJUWQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
--
Best regards.
Yukang
|
Hmm.. You have a good point. We should upgrade to the latest and see what needs to be changed. I will spend sometime to at least get a sense. |
Upgrading oci-spec-rs is tracked at this issue: containers/youki#225 |
I have more investigation on this issue. If we need to move out of this repo, we need make those fields public. Anyway, I updated my this PR with latest of oci-spec-rs. @utam0k |
@utam0k
It seems the not builder version of oci-spec-rs. If I work based on this, we need to more work since the latest oci-spec-rs has been moved to the builder version. Anyone is working to update oci-spec-rs to latest for youki? |
@chenyukang Hi. I upgraded oci-spec-rs for youki. containers/youki#303 |
Oh, thanks for reminding! I forget update my local repo with upstream. 😂 |
We don't necessarily need to make the fields public. Take a look at how |
Thanks! I will have a look. |
If we want to modify some fields on a Struct, we must clone the left fields of it like this? let mut spec_builder = SpecBuilder::default()
.process(process)
.version(spec.version())
.linux(linux);
spec_builder = match spec.root() {
Some(root) => spec_builder.root(root.clone()),
None => spec_builder,
};
spec_builder = match spec.mounts() {
Some(mounts) => spec_builder.mounts(mounts.clone()),
None => spec_builder,
};
spec_builder = match spec.hostname() {
Some(hostname) => spec_builder.hostname(hostname.clone()),
None => spec_builder,
};
spec_builder = match spec.hooks() {
Some(hooks) => spec_builder.hooks(hooks.clone()),
None => spec_builder,
};
spec_builder = match spec.annotations() {
Some(annotations) => spec_builder.annotations(annotations.clone()),
None => spec_builder,
}; |
For the moment, that's the only way. That's the price we pay for making the struct immutable, which is a good trade off. The right solution to remove a lot of this boilerplate is to implement From trait allowing change from a Spec to a SpecBuilder. After that, we can refactor both the containers/youki#351 tracking |
Closing this PR since we implement this option in youki: containers/youki#350 |
No description provided.