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

Refactor CI #2012

Merged
merged 2 commits into from
Jun 7, 2023
Merged

Refactor CI #2012

merged 2 commits into from
Jun 7, 2023

Conversation

yihuaf
Copy link
Collaborator

@yihuaf yihuaf commented Jun 6, 2023

  • attempt to standardized all CI to use Ubuntu 22 (the integration validation using runc is currently failing on Ubuntu 22, but others are successful)
  • refactored the justfile to be more ergonomics
  • moved the rust integration test to e2e
  • moved the oci validation go version to e2e
  • change the file filter action to tj-actions/changed-files
  • refactored the dependencies installation to just prepare recipe
  • update readme to replace make related calls.

Fix #1974

CI should all pass. Note the integration_test is in a Expected state. This is due to branch protection rules and not directly related to this PR. In this PR, we renamed the integration_test to oci-validation-go. When moving to e2e.yaml, the name integration_test is way too ambiguous. We will fix the branch protection rule once the PR is merged into main or right after approve of this PR.

@YJDoc2 YJDoc2 self-requested a review June 6, 2023 05:35
@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2023

Codecov Report

Merging #2012 (7818895) into main (2ff8b97) will decrease coverage by 0.36%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2012      +/-   ##
==========================================
- Coverage   65.26%   64.90%   -0.36%     
==========================================
  Files         129      129              
  Lines       14802    14802              
==========================================
- Hits         9660     9607      -53     
- Misses       5142     5195      +53     

@yihuaf yihuaf closed this Jun 6, 2023
@yihuaf yihuaf reopened this Jun 6, 2023
@yihuaf yihuaf self-assigned this Jun 6, 2023
@yihuaf yihuaf marked this pull request as ready for review June 6, 2023 08:13
@yihuaf yihuaf force-pushed the yihuaf/ci branch 2 times, most recently from b7bbdb0 to d63d467 Compare June 6, 2023 08:29
@yihuaf yihuaf requested review from utam0k and a team June 6, 2023 08:29
@yihuaf
Copy link
Collaborator Author

yihuaf commented Jun 6, 2023

@YJDoc2 This is ready :)

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, one of the issue I am facing is with the prepare recipe. Here we have moved all dependency install in that, and run it to setup ci env. However, we only check for plain ubuntu and fedora ; which is too restrictive. For example, this fails on Pop OS, which is ubuntu derivative. What I think would be better is to leave the general setup instructions as they are right now (so readme will still show apt/dnf install commands), and make a recipe tailored to ubuntu which will be use only to setup CI env.

Also, maybe rename it to 'setup-ci-env' instead of prepare...

README.md Outdated Show resolved Hide resolved
@yihuaf
Copy link
Collaborator Author

yihuaf commented Jun 6, 2023

@YJDoc2 Thanks for the quick review. I understand the concern you raised regarding to the setup. I will restore the instruction back for now and link to the just git repo. I will propose another PR so this point won't block the rest of the PR.

Here is my rationale. There are two approaches when it comes to setting up environment. One, as you suggested, is to link/provide information in the readme and/or other source of readings, and let users decide how to proceed. The other approach is to make it as close to a single button click setup as possible using automation. In this approach, the goal is that users don't have to figure out what packages to install and where to download just. This is what a prepare target tries to achieve. The target provides an opinionated way that works. This has the added benefit of single point of truth, where adding one more dependencies doesn't require changing a bunch of other places, like we do today. There will be no out of date instructions because the instruction is coded up and checked by CI.

Here is what I propose. We declare that Ubuntu and Fedora are the official dev environment we support. We will create automation for setting up the environment for these two OS. Eventually, we would even make CI a matrix strategy running both Ubuntu and Fedora.

For all other dev environment, we can point them to the Ubuntu/Fedora scripts/recipe and let people figure out what needs to be done for their specific environment. This probably provides a balance between automation in the fast path while do not neglect the slow path.

- standarized all CI to use Ubuntu 22
- refactored the justfile
- moved the rust integration test to e2e
- moved the oci validation go version to e2e
- change the file filter action to `tj-actions/changed-files`
- refactored the dependencies installation to `just prepare` recipe
- fix readme (make -> just)
- fix readme minor lint issuefix

Signed-off-by: yihuaf <[email protected]>
Signed-off-by: yihuaf <[email protected]>
@yihuaf yihuaf requested a review from YJDoc2 June 6, 2023 18:47
@yihuaf
Copy link
Collaborator Author

yihuaf commented Jun 6, 2023

@YJDoc2 PTAL

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jun 7, 2023

Hey @yihuaf , I think the idea of officially supported distributions and whether to have a one-step setup process or not needs more discussion, as they are pretty big changes for all users/developers who want to build/setup youki. I think what you have suggested definitely has a merit and if I remember correctly it was once discussed when issue numbers were in ~300 ...
So for now, I'll merge this PR in its current state as it is lgtm and does the work of refactoring the ci as intended. I'll open a new Issue later today to discuss about the official supported distros etc. Given that it can be a big impacting decision (to make certain distros "officially" supported) , I think we should definitely wait for utam0k to get better and hear his opinion on it as well.

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 YJDoc2 merged commit 93a0afd into youki-dev:main Jun 7, 2023
@yihuaf yihuaf deleted the yihuaf/ci branch June 7, 2023 05:33
@yihuaf
Copy link
Collaborator Author

yihuaf commented Jun 7, 2023

Hey @yihuaf , I think the idea of officially supported distributions and whether to have a one-step setup process or not needs more discussion, as they are pretty big changes for all users/developers who want to build/setup youki. I think what you have suggested definitely has a merit and if I remember correctly it was once discussed when issue numbers were in ~300 ... So for now, I'll merge this PR in its current state as it is lgtm and does the work of refactoring the ci as intended. I'll open a new Issue later today to discuss about the official supported distros etc. Given that it can be a big impacting decision (to make certain distros "officially" supported) , I think we should definitely wait for utam0k to get better and hear his opinion on it as well.

I agree. Btw, thank you for pointing this out. I do realize now that this has a bigger impact than I originally anticipated. I am used to making dev environment as automated and consistent as possible, because that is how it is done when you work for a company. The priorities are how to onboard someone quickly and keeping things as consistent as possible. Here, the priorities are different where we should accommodate to a wide number of audiences with many different dev setups. We will continue the discussion in the other issue as you suggest.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Jun 7, 2023

I agree. Btw, thank you for pointing this out. I do realize now that this has a bigger impact than I originally anticipated. I am used to making dev environment as automated and consistent as possible, because that is how it is done when you work for a company. The priorities are how to onboard someone quickly and keeping things as consistent as possible. Here, the priorities are different where we should accommodate to a wide number of audiences with many different dev setups. We will continue the discussion in the other issue as you suggest.

No worries! Even I noticed it because it was discussed once before, and I had read discussions similar to this in some other open source s/w , where they wanted to decide what should be their official supported platforms :) Also, I can understand the ease of having an autometed setup and consistent base ; its just that as you said correctly, we also need to consider that our audience is wider, and hacks stuff with varied setup. (I think there was recently an issue opened for dind and capabilites)

fyi, I have updated the branch protection rules for the new ci name as you have suggested.

I'll open the separate issue for this discussion later today, will also link the old issue I mentioned and some other pov I have in mind.

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.

Need to add integration test to the regular CI runs, not just integration_validation
3 participants