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

Add test process user #2978

Merged
merged 23 commits into from
Nov 14, 2024
Merged

Add test process user #2978

merged 23 commits into from
Nov 14, 2024

Conversation

sat0ken
Copy link
Contributor

@sat0ken sat0ken commented Nov 4, 2024

This implements the process_user validation in #361

@YJDoc2 YJDoc2 mentioned this pull request Nov 5, 2024
Open
60 tasks
YJDoc2
YJDoc2 requested changes Nov 6, 2024
View reviewed changes
tests/contest/runtimetest/src/tests.rs Outdated Show resolved Hide resolved
tests/contest/runtimetest/src/utils.rs Outdated Show resolved Hide resolved
tests/contest/contest/src/tests/process_user/process_user.rs Outdated Show resolved Hide resolved
@sat0ken
rename module name
ab34d99 
Signed-off-by: sat0ken <[email protected]>
@sat0ken
fix unsafe code to use nix
df941cd 
Signed-off-by: sat0ken <[email protected]>
@sat0ken
fix err to use nix
2fe523b 
Signed-off-by: sat0ken <[email protected]>
@sat0ken
fix format err
ce496b6 
Signed-off-by: sat0ken <[email protected]>
@sat0ken
Copy link
Contributor Author

sat0ken commented Nov 10, 2024

@YJDoc2 HI, I've fixed the code according to the comments. Could you please check the code?

YJDoc2
YJDoc2 requested changes Nov 11, 2024
View reviewed changes
Copy link
Collaborator

@YJDoc2 YJDoc2 left a comment

Choose a reason for hiding this comment

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

some nits, some changes. Please take a look.

tests/contest/contest/src/tests/process_user/process_user_test.rs Outdated Show resolved Hide resolved
tests/contest/runtimetest/src/tests.rs Show resolved Hide resolved
tests/contest/runtimetest/src/tests.rs Outdated Show resolved Hide resolved
tests/contest/runtimetest/src/tests.rs Outdated Show resolved Hide resolved
tests/contest/runtimetest/src/tests.rs Outdated Show resolved Hide resolved
tests/contest/contest/src/tests/process_user/process_user_test.rs Outdated Show resolved Hide resolved
@sat0ken
Copy link
Contributor Author

sat0ken commented Nov 13, 2024

@YJDoc2 Thank you for review and comment! I fixed code, Could you please check the code?

YJDoc2
YJDoc2 reviewed Nov 13, 2024
View reviewed changes
Copy link
Collaborator

@YJDoc2 YJDoc2 left a comment

Choose a reason for hiding this comment

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

Hey, some final nitpicks, please check them. Otherwise good 👍

tests/contest/contest/src/tests/process_user/process_user_test.rs Outdated Show resolved Hide resolved
tests/contest/runtimetest/src/tests.rs Outdated Show resolved Hide resolved
tests/contest/runtimetest/src/tests.rs Outdated Show resolved Hide resolved
@sat0ken
Copy link
Contributor Author

sat0ken commented Nov 13, 2024

@YJDoc2 Thank you for review and comment! I fixed code, Could you please check the code?

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Nov 14, 2024

Hey I had a very minor change in the error message , so I did in and pushed. Hope that is ok. Apart from that the PR is good to go so I'll merge. Thanks for your contribution :)

YJDoc2
YJDoc2 approved these changes Nov 14, 2024
View reviewed changes
Copy link
Collaborator

@YJDoc2 YJDoc2 left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Thanks!

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Nov 14, 2024

ohh it seems we have found a bug in youki : https://github.com/youki-dev/youki/actions/runs/11838247800/job/32986965984?pr=2978

Which means your code is working nicely and helping!

From youki side, I'll need to see why providing duplicate gid works when it shouldn't. From tests side I'm letting this go and merge for now, but in my follow up PR on this, I'll update this test.

@YJDoc2 YJDoc2 merged commit d5d5fad into youki-dev:main Nov 14, 2024
27 checks passed
@github-actions github-actions bot mentioned this pull request Nov 14, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YJDoc2 YJDoc2 YJDoc2 approved these changes

Assignees
No one assigned
Labels
kind/test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sat0ken @YJDoc2