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 github action cache for redis-server binary #372

Merged

Conversation

wszaranski
Copy link
Contributor

${MAKE} -C integration redis_src/redis-server that is part of ci target takes around 2 minutes to execute. With addition of cache preventing rebuild of redis-server tests run time drops from ~2m50s to ~50s.

Currently github actions are free for public repositories but I think that shorter execution time will improve developer experience :)

Due to matrix build for go versions, when redis is upgraded, build will be run multiple times in parallel. Unnecessary parallel builds can be avoided but since we don't upgrade redis too often I feel it would be unnecessary complication.

Perhaps even better solution might be usage of testcontainers. Integration tests would basically download and run official redis docker image and spin it up for tests. There would be no need for cache or building binary but docker dependency would be introduced. I have decided to introduce cache because docker dependency has pros and cons that need to be discussed while cache is clear and easy win ;)

wszaranski added a commit to wszaranski/miniredis that referenced this pull request Apr 9, 2024
${MAKE} -C integration redis_src/redis-server that is part of ci target
takes around 2 minutes to execute. With addition of cache preventing
rebuild of redis-server tests run time drops from ~2m50s to ~50s.

Currently github actions are free for public repositories[1] but I think
that shorter execution time will improve developer experience :)

Due to matrix build for go versions, when redis is upgraded, build will be
run multiple times in parallel. Unnecessary parallel builds can be
avoided but since we don't upgrade redis too often I feel it would be
unnecessary complication.

Perhaps even better solution might be usage of testcontainers[2].
Integration tests would basically download and run official redis docker
image and spin it up for tests. There would be no need for cache or building
binary but docker dependency would be introduced. I have decided to
introduce cache because docker dependency has pros and cons that need to
be discussed while cache is clear and easy win ;)

PR: alicebob#372
[1]: https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration
[2]: https://golang.testcontainers.org/

Signed-off-by: Wojciech Szarański <wojciech.szaranski@gmail.com>
@wszaranski wszaranski force-pushed the miniredis-add-redis-server-binary-cache branch from 34d6185 to 11f666b Compare April 9, 2024 18:02
${MAKE} -C integration redis_src/redis-server that is part of ci target
takes around 2 minutes to execute. With addition of cache preventing
rebuild of redis-server tests run time drops from ~2m50s to ~50s.

Currently github actions are free for public repositories[1] but I think
that shorter execution time will improve developer experience :)

Due to matrix build for go versions, when redis is upgraded, build will be
run multiple times in parallel. Unnecessary parallel builds can be
avoided but since we don't upgrade redis too often I feel it would be
unnecessary complication.

Perhaps even better solution might be usage of testcontainers[2].
Integration tests would basically download and run official redis docker
image and spin it up for tests. There would be no need for cache or building
binary but docker dependency would be introduced. I have decided to
introduce cache because docker dependency has pros and cons that need to
be discussed while cache is clear and easy win ;)

PR: alicebob#372
[1]: https://docs.github.com/en/actions/learn-github-actions/usage-limits-billing-and-administration
[2]: https://golang.testcontainers.org/

Signed-off-by: Wojciech Szarański <wojciech.szaranski@gmail.com>
@wszaranski wszaranski force-pushed the miniredis-add-redis-server-binary-cache branch from 11f666b to be85aa3 Compare April 9, 2024 18:08
@wszaranski
Copy link
Contributor Author

Difference can be observed between run when cache wasn't available (2m44s) and when it was available (49s).

@wszaranski
Copy link
Contributor Author

wszaranski commented Apr 9, 2024

I think that it would be funny to use competing solution (testcontainers) to run miniredis integration tests.

Miniredis is smaller, way faster and doesn't require external dependency (docker) which is great ❤️ but maybe we can mention testcontainers in README for people that require features not implemented in miniredis?

@alicebob alicebob merged commit 477fc38 into alicebob:master Apr 9, 2024
4 checks passed
@alicebob
Copy link
Owner

alicebob commented Apr 9, 2024

Fun, thanks!

Testcontainers look fun, but there are similar things which also work well (for example https://devenv.sh/ ), so I think it's a bit out of scope to pick something. But then again a short "Other options" (or whatever) section in the readme is fine with me :)

@wszaranski wszaranski deleted the miniredis-add-redis-server-binary-cache branch April 10, 2024 12:53
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.

None yet

2 participants