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

Introduce a workspace to enable execution of commands in bulk. #287

Merged
merged 6 commits into from
Sep 14, 2021

Conversation

utam0k
Copy link
Member

@utam0k utam0k commented Sep 9, 2021

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2021

Codecov Report

Merging #287 (8049ba4) into main (9844d84) will increase coverage by 3.89%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #287      +/-   ##
==========================================
+ Coverage   73.36%   77.26%   +3.89%     
==========================================
  Files          46       42       -4     
  Lines        6229     5920     -309     
==========================================
+ Hits         4570     4574       +4     
+ Misses       1659     1346     -313     

@utam0k utam0k force-pushed the improvement/workspace-members branch from a4c6150 to f303123 Compare September 9, 2021 14:07
@utam0k utam0k marked this pull request as ready for review September 9, 2021 14:11
@utam0k utam0k requested a review from YJDoc2 September 9, 2021 14:12
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 👍
I think as workspace makes the /target dir common for both (youki and cgroups), so we might be able to remove the separate cache action for cgroups as well, will have to check that.

In cgroups I think you were trying to extract common statement outside if-else, but it might create issues for #266 ? It is already having issues with merge conflicts with existing files, so I feel it'd be better to leave those changes for @guni1192 to add in their PR. What do you think?

@Furisto
Copy link
Collaborator

Furisto commented Sep 9, 2021

You can leave it out. My open PR contains the same change, so it will not get lost.

@Furisto
Copy link
Collaborator

Furisto commented Sep 9, 2021

Oh wait, I don't think this will work, because with the workspace clippy will flag the code now.

@utam0k
Copy link
Member Author

utam0k commented Sep 10, 2021

@Furisto @YJDoc2
Thank you for your comments. Well, I don't want to bother @guni1192 too much since #266 is a big change, so I'll draft this PR and wait for it to be merged.

@utam0k utam0k marked this pull request as draft September 10, 2021 00:49
@utam0k utam0k marked this pull request as ready for review September 12, 2021 08:34
@utam0k
Copy link
Member Author

utam0k commented Sep 12, 2021

@Furisto @YJDoc2 PTAL

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Sep 12, 2021

Additionally we can remove the Cargo.lock file in cgroups, and combine the separate cache actions, as the compiled cgroups files are kept in the youki's target/ only.

https://doc.rust-lang.org/book/ch14-03-cargo-workspaces.html

A workspace is a set of packages that share the same Cargo.lock and output directory.

Other than that lgtm 👍

@utam0k
Copy link
Member Author

utam0k commented Sep 13, 2021

Additionally we can remove the Cargo.lock file in cgroups, and combine the separate cache actions, as the compiled cgroups files are kept in the youki's target/ only.

https://doc.rust-lang.org/book/ch14-03-cargo-workspaces.html

A workspace is a set of packages that share the same Cargo.lock and output directory.

Other than that lgtm

@YJDoc2 Oh thanks for the review. I've taken care of it.

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 👍 if you want to keep cache actions separate for now.

@utam0k
Copy link
Member Author

utam0k commented Sep 14, 2021

lgtm if you want to keep cache actions separate for now.

We could certainly organize it. I'll consider it in another PR.

@utam0k utam0k merged commit 40e8363 into youki-dev:main Sep 14, 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.

4 participants