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

Add new method to instantiate Delete command #262

Merged
merged 1 commit into from
Sep 20, 2021
Merged

Conversation

rosds
Copy link
Contributor

@rosds rosds commented Sep 3, 2021

We were trying to use youki as a library and notice that the Delete command was missing the new method. It can not be instantiated otherwise. This patch also improves the api consistency with other commands.

@rosds rosds marked this pull request as ready for review September 3, 2021 13:17
@utam0k
Copy link
Member

utam0k commented Sep 3, 2021

@alfonsoros88 Thanks for your contribution! There are new() because only Create and Start are special, and there are used for Run. youki's commands aren't currently planned to be provided as a library, but what do you plan to use them for?

@codecov-commenter
Copy link

codecov-commenter commented Sep 3, 2021

Codecov Report

Merging #262 (1b14191) into main (3e242b9) will increase coverage by 34.27%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main     #262       +/-   ##
===========================================
+ Coverage   39.85%   74.13%   +34.27%     
===========================================
  Files          15       43       +28     
  Lines        1405     6596     +5191     
  Branches      385      385               
===========================================
+ Hits          560     4890     +4330     
- Misses        665     1630      +965     
+ Partials      180       76      -104     

@utam0k utam0k closed this Sep 19, 2021
@rosds
Copy link
Contributor Author

rosds commented Sep 20, 2021

@utam0k sorry for the delay! We were trying to use youki's commands by including it as a library dependency to our project. We thought it would be easier to handle it that way instead of using the youki binary and having to handle the dependency with a build.rs.

For creating containers we are not using the Create command directly, as it is just a wrapper of ContainerBuilder. Unfortunately, for cleaning up, the Delete command seems to be the one containing the logic for removing containers. Instead of copy/pasting the exec function, we thought that we could just use the Delete command directly and for that we needed the new() method.

@utam0k utam0k reopened this Sep 20, 2021
@utam0k
Copy link
Member

utam0k commented Sep 20, 2021

@alfonsoros88
I see I understand. However, I don't really want to think about providing anything under src/ as a library. Is there any problem to generate Delete structure directly instead?

Delete {
  contaienr_id,
  force,
}

@rosds
Copy link
Contributor Author

rosds commented Sep 20, 2021

@utam0k we are trying to use youki in https://github.com/esrlabs/northstar. This project uses its own "kind" of containers, we wanted to add support for OCI containers by using youki underneath.

It is not possible to instantiate Delete in that way because the fields are not public.

@utam0k
Copy link
Member

utam0k commented Sep 20, 2021

@utam0k we are trying to use youki in https://github.com/esrlabs/northstar. This project uses its own "kind" of containers, we wanted to add support for OCI containers by using youki underneath.

It is not possible to instantiate Delete in that way because the fields are not public.

@alfonsoros88 I understand. Let's provide it. Could you add a comment to the effect that you are providing new() for those who are not using it as youki, but are using youki as a library?

@rosds
Copy link
Contributor Author

rosds commented Sep 20, 2021

@utam0k sure thing! Do you refer to a commit comment or as a doc comment for the new() method?

@utam0k
Copy link
Member

utam0k commented Sep 20, 2021

@utam0k sure thing! Do refer to a commit comment or as a doc comment for the new() method?

Please do so in the doc comments.

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.

Thanks!

@utam0k utam0k merged commit 13fe16f into youki-dev:main Sep 20, 2021
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.

3 participants