search

DiceDB / dice

DiceDB is a redis-compliant, reactive, scalable, highly-available, unified cache optimized for modern hardware.
https://dicedb.io/
Other
6.88k stars 1.1k forks source link

A Better CI #1116

Closed kitarp29 closed 3 weeks ago

kitarp29 commented 1 month ago

Hi @JyotinderSingh I have made some changes as per the disccusion on issue:

Changes:

  1. The dockerhub.yml is going to cache layers to resuse. I have some questions around this one. Since I see this CI will only trigger on release it means frequency will be less. I have added the changes, but couldn't test it. Please go through it once, then we can talk more on it.

  2. The full-test-suite.yml is going to cache the go dependencies from last build. Only pull new if required. Now I see Suryavir has very well defined to ignore the files when not to run the CI. We can make it run only on changes to go files or go.mod. Because with time, my understanding is that new folders and types of files may be included.

  3. The linter.yml now will only run when a change is done to a .go file.

  4. The cve-scan.yml I am using Trivy to find the CVEs in the codebase. I have kept it to run once a week and on demand. I need to understand the requirement on this one from you as well

Other Discussions:

  1. An Image scan can be done on Github actions with Trivy or anyone other options on Github marketplace. Also Docker has scout which does it for you on DockerHub as well. You let me know which is preferred by the team.

  2. I understand caching the go dependencies on full-test-suite is a lil overkill. Since pulling Go pkgs is free. I think it can just decrease the build time a lil. But it's your call to keep or remove

Sorry for the delay on this one. My personal laptop was broken and I was on a little vaccation. cc: @arpitbbhayani

lucifercr07 commented 1 month ago

@kitarp29 can you please check the review comments once

kitarp29 commented 1 month ago

@lucifercr07 What about the rest of the changes? Do you agree we should have all of my changes in the CI

JyotinderSingh commented 3 weeks ago

LGTM, Thanks for the changes @kitarp29! Thanks for the review @lucifercr07.

Additionally, wondering if we can improve this further by introducing conditional triggering for tests (we don't need to run integration and unit tests if someone is only modifying the docs or the readme). We can take it up in a new PR.

JyotinderSingh commented 3 weeks ago

We can merge once the existing comment is resolved.

kitarp29 commented 3 weeks ago

@JyotinderSingh first thing is I just want @lucifercr07 confirmation on the last comment, I will implement something like that.

As for the conditional testing, I had asked about it at the start itself:

The full-test-suite.yml is going to cache the go dependencies from last build. Only pull new if required. Now I see Suryavir has very well defined to ignore the files when not to run the CI. We can make it run only on changes to go files or go.mod. Because with time, my understanding is that new folders and types of files may be included.

lucifercr07 commented 3 weeks ago

@lucifercr07 What about the rest of the changes? Do you agree we should have all of my changes in the CI

@kitarp29 looks good to me, please let me know once ready to merge.

kitarp29 commented 3 weeks ago

@JyotinderSingh I think we can merge this PR for now. I will raise another issue for the two features we all agree could help

  1. Conditional triggering of linting and Testing pipeline when changes done to only .go file (This can change if the project ever changes/includes anything other than go)
  2. Caching setup for the linting job as well cc: @lucifercr07
JyotinderSingh commented 3 weeks ago

Thanks for the contribution!