Skip to content
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.

Sign up for GitHub

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

Jump to bottom

adding HOME into envs when init containers #681

Merged
merged 8 commits into from
Feb 8, 2022
Merged

adding HOME into envs when init containers #681

merged 8 commits into from
Feb 8, 2022

Conversation

mitnk
Copy link
Contributor

@mitnk mitnk commented Feb 5, 2022

closing #680

@mitnk mitnk mentioned this pull request Feb 5, 2022
Closed
@mitnk mitnk changed the title WIP: adding HOME into envs when init containers adding HOME into envs when init containers Feb 6, 2022
mitnk
mitnk commented Feb 6, 2022
View reviewed changes
build.sh
cd ./runtimetest
cargo build --verbose $TGT $1
cargo build $TGT $1 $2
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the output of --verbose is too verbose in regular developing activity. I think we can change to use them as needed. e.g.

$ ./build.sh
$ ./build.sh --verbose
$ ./build.sh --release
$ ./build.sh --verbose --release

a bit dirty but works. we can make this script mature if need later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@codecov-commenter
Copy link

Codecov Report

Merging #681 (8c65bc8) into main (a8e4003) will decrease coverage by 0.18%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #681      +/-   ##
==========================================
- Coverage   69.65%   69.46%   -0.19%     
==========================================
  Files          84       84              
  Lines       10990    11018      +28     
==========================================
- Hits         7655     7654       -1     
- Misses       3335     3364      +29

utam0k
utam0k reviewed Feb 6, 2022
View reviewed changes
docs/src/user/basic_usage.md
@@ -56,6 +56,26 @@ Then to start the original/normal Docker daemon, you can run
sudo systemctl start docker
```

#### Let docker permanently know youki as a runtime
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

utam0k
utam0k reviewed Feb 6, 2022
View reviewed changes
Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I ask you to add the unit test?

@mitnk
Copy link
Contributor Author

mitnk commented Feb 6, 2022

Can I ask you to add the unit test?

Sure, do I have an existing test to refer on?

@utam0k
Copy link
Member

utam0k commented Feb 6, 2022

Can I ask you to add the unit test?

Sure, do I have an existing test to refer on?

If you search the code with mod tests, you can see various unit tests. If it looks too difficult, I'll send out a PR to add it, and you can review it.

@mitnk
Copy link
Contributor Author

mitnk commented Feb 6, 2022

The code is ready for another check.

Furisto
Furisto reviewed Feb 6, 2022
View reviewed changes
Copy link
Collaborator

@Furisto Furisto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @mitnk, you need to sign your commits in order to pass CI.

@mitnk
WIP: adding HOME into envs when init containers
8d9a1f9 
Signed-off-by: Hugo Wang <w@mitnk.com>
@mitnk
update the usage of getpwuid_r
4f1f273 
Signed-off-by: Hugo Wang <w@mitnk.com>
@mitnk
check if HOME already exists
7a467b4 
Signed-off-by: Hugo Wang <w@mitnk.com>
@mitnk
misc updates
45df04b 
Signed-off-by: Hugo Wang <w@mitnk.com>
@mitnk
misc updates
86e26ad 
Signed-off-by: Hugo Wang <w@mitnk.com>
@mitnk
update build.sh
19cd05d 
Signed-off-by: Hugo Wang <w@mitnk.com>
@mitnk
use nix::User instead
489d173 
Signed-off-by: Hugo Wang <w@mitnk.com>
@mitnk
fix fmt
a7f20e8 
Signed-off-by: Hugo Wang <w@mitnk.com>
@mitnk
Copy link
Contributor Author

mitnk commented Feb 7, 2022

rebased for sign-off.

@Furisto
Copy link
Collaborator

Furisto commented Feb 8, 2022

@mitnk Thanks!

@Furisto Furisto merged commit 62a10c6 into youki-dev:main Feb 8, 2022
@mitnk mitnk deleted the env-home branch February 9, 2022 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@utam0k utam0k utam0k left review comments

@Furisto Furisto Furisto approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mitnk @codecov-commenter @utam0k @Furisto