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.

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

youki exec should not clean up on error #1818

Merged
merged 2 commits into from
Apr 14, 2023
Merged

Conversation

yihuaf
Copy link
Collaborator

@yihuaf yihuaf commented Apr 14, 2023

Fix #1810

Signed-off-by: yihuaf <yihuaf@unkies.org>
@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2023

Codecov Report

Merging #1818 (a822e88) into main (1e894a0) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1818      +/-   ##
==========================================
- Coverage   68.64%   68.62%   -0.03%     
==========================================
  Files         121      121              
  Lines       13325    13327       +2     
==========================================
- Hits         9147     9145       -2     
- Misses       4178     4182       +4     

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Apr 14, 2023

Hey @yihuaf , what are your thoughts on moving the if-check inside match, instead of using it as a match guard ? There is no particular benefit to doing one instead of other, but I feel that it might more sense to move it inside like :

Ok(pid)=>...
Err(outer)=>{
     if matches!(....){
          // cleanup, if that errors , return outer.context(inner)
     }
     Err(outer)
}

I don't know if this will be better, so giving lgtm, because looks good 👍

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 👍

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.

May I ask you to write the integration test for this case?

@yihuaf
Copy link
Collaborator Author

yihuaf commented Apr 14, 2023

May I ask you to write the integration test for this case?

Yes I was thinking about the same.

Signed-off-by: yihuaf <yihuaf@unkies.org>
@yihuaf
Copy link
Collaborator Author

yihuaf commented Apr 14, 2023

Merge this PR as is. I created #1819 to track the integration test. We have a few related bugs in this areas, so I want to go over the container life cycle tests with a new PR.

@yihuaf yihuaf merged commit a760731 into youki-dev:main Apr 14, 2023
@yihuaf yihuaf deleted the yihuaf/1810 branch April 14, 2023 15:03
@utam0k
Copy link
Member

utam0k commented Apr 14, 2023

@yihuaf 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Youki exec a does not exist process will stop running container
4 participants