-
Notifications
You must be signed in to change notification settings - Fork 939
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
chore(gh-workflows): add regression tests to release job #1665
Conversation
8374185
to
39922e9
Compare
@royjacobson can you please take a look? |
@@ -32,64 +29,16 @@ jobs: | |||
- name: Configure & Build | |||
run: | | |||
apt update && apt install -y pip jq | |||
cmake -B ${GITHUB_WORKSPACE}/build -DCMAKE_BUILD_TYPE=${{matrix.build-type}} -GNinja \ | |||
cmake -B ${GITHUB_WORKSPACE}/build-opt -DCMAKE_BUILD_TYPE=${{matrix.build-type}} -GNinja \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason we have not called it opt because we have a matrics that build debug build as well.
what's the reason you had to rename?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I don't have to pass the build directory as an argument to the action. Since there are dependencies, I will simply add a parameter to the action
Very nice that you factored out the common code. |
@@ -104,7 +104,7 @@ jobs: | |||
submodules: true | |||
- name: Configure | |||
run: | | |||
apt update && apt install -y debhelper moreutils | |||
apt update && apt install -y debhelper moreutils pip jq |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we now have several "apt install" calls in our actions. Let's eliminate them by adding all these packages to ghcr.io/romange/ubuntu-dev
. Feel free to send a PR to https://github.com/romange/container-foundry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already opened a PR.
- I would like to add regression tests for arm as well. I know I said offline that it doesn't worth our time, but I am already familiar with most of the gh action staff now that I should be able to find a good solution (and my OCD won't just let me to only run it for x86). The issue is that I could not run the regression tests action as part of the
run-on-aarch
action. I could simply not find a way to do this properly. Moreover, we only have x86 runners so running on a pure aarch machine was a big no and therefore we require QEMU. But as I looked on your repo, it seems that you already using QEMU which means that I could reuse your images instead ofrun-on-aarch
and therefore I would be able to run that action as separate step (because the OG issue, that I can't run an action in the context of another action, that is, therun-on-aarch
). - As I said I made this PR, but I can clean up the release even more once I get the regression tests running on a container explicitly (as opposed to the action). I won't wait for my PR to be merged. I will open a new one to clean up everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/dragonflydb/dragonfly/pull/1706/files
- I refactored to use your QEMU docker image instead of the run-on-aarch action
- I refactored to use a matrix since both build-native and build-qemu are almost identical (the differences are now arguments to the matrix)
- I run the regression tests on aarh as well :)
plz release a new package to your repo such that I can remove the apt install's
LGTM, please fix Roman's comment about running the tests in debug mode :) |
3c48454
to
9f98f1c
Compare
f4f5b3c
to
f256e6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Addresses #1625