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

Refactored the Executor interface yet again #2230

Merged
merged 3 commits into from
Aug 6, 2023

Conversation

yihuaf
Copy link
Collaborator

@yihuaf yihuaf commented Aug 3, 2023

I apologize having to refactor this interface yet again. Previously, we introduced the executor to be a function pointer. This works out nicely because the function pointer in rust can be clone-ed without hassel. However, I realized that using function pointer is way too restrictive for our users. The executor may wish to include additional context when calling the exec function. The function pointer limited the input only oci spec. The new constraint is that the struct implementing executor must be clone-able, which we need to carry this struct across process boundry.

I apologize having to refactor this interface yet again. Previously, we
introduced the executor to be a function pointer. This works out nicely
because the function pointer in rust can be clone-ed without hassel.
However, I realized that using function pointer is way to restrictive
for our users. The executor may wish to include additional context when
calling the exec function. The function pointer limited the input only
`oci spec`.

Signed-off-by: yihuaf <[email protected]>
@yihuaf yihuaf added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. breaking change labels Aug 3, 2023
@yihuaf yihuaf self-assigned this Aug 3, 2023
@yihuaf yihuaf requested review from utam0k, YJDoc2 and a team August 3, 2023 04:51
Signed-off-by: yihuaf <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2023

Codecov Report

Merging #2230 (009eae7) into main (0fc4c87) will decrease coverage by 0.03%.
Report is 16 commits behind head on main.
The diff coverage is 6.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2230      +/-   ##
==========================================
- Coverage   65.03%   65.01%   -0.03%     
==========================================
  Files         129      129              
  Lines       15139    15149      +10     
==========================================
+ Hits         9846     9849       +3     
- Misses       5293     5300       +7     

Signed-off-by: yihuaf <[email protected]>
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.

looks good 👍
I'm going to squash and merge so that the doc test and spell fixing commits get merged together.
If I'm not wrong, then this is basically the original executor scheme except for the executor manager and executor lists, right?

@YJDoc2 YJDoc2 merged commit bda9370 into youki-dev:main Aug 6, 2023
13 checks passed
@yihuaf
Copy link
Collaborator Author

yihuaf commented Aug 8, 2023

If I'm not wrong, then this is basically the original executor scheme except for the executor manager and executor lists, right?

Yes. I also removed the can_handle and the executor name. The executor name is removed because this doesn't apply to the new scheme. The can_handle part of the logic is moved into exec with an error kind can't handle. With that being said, there are some talks to add the can_handle or a validate function back, so we can abstract the validation part of the logic out for each executor.

@yihuaf yihuaf deleted the yihuaf/executor branch August 8, 2023 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants