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

CI: Add Support for Valkey 6.2, 7.0 and 7.2 #1562

Closed
wants to merge 18 commits into from

Conversation

barshaul
Copy link
Collaborator

@barshaul barshaul commented Jun 13, 2024

CI: Add Support for Valkey 7.2.5

This PR introduces Valkey 7.2.5 to the CI matrix tests. Specifically, it adds Valkey to the matrix for the main code integration tests across all wrappers.

Included in this PR:

  • Integration of Valkey 7.2.5 in the main code integration tests for all wrappers.

Not included in this PR:

  • Integration of Valkey 7.2.5 for other tests such as:
    • Building on Amazon Linux 2
    • Building on macOS for Python and Node
    • Continuous Deployment (CD) actions

These additional integrations can be addressed in a follow-up PR.

@barshaul barshaul force-pushed the valkey_ci branch 9 times, most recently from 13a5ab0 to 9fbf1d7 Compare June 13, 2024 18:25
@barshaul barshaul changed the title ##WIP## - CI: Adding support in Valkey ##WIP## - CI: Add Support for Valkey 7.2.5 Jun 13, 2024
@barshaul barshaul marked this pull request as ready for review June 13, 2024 18:34
@barshaul barshaul requested a review from a team as a code owner June 13, 2024 18:34
@barshaul barshaul changed the title ##WIP## - CI: Add Support for Valkey 7.2.5 CI: Add Support for Valkey 7.2.5 Jun 13, 2024
@Yury-Fridlyand Yury-Fridlyand added the CI/CD CI/CD related label Jun 13, 2024
.github/workflows/csharp.yml Outdated Show resolved Hide resolved
.github/workflows/go.yml Outdated Show resolved Hide resolved
.github/workflows/install-shared-dependencies/action.yml Outdated Show resolved Hide resolved
.github/workflows/install-shared-dependencies/action.yml Outdated Show resolved Hide resolved
Comment on lines +8 to +18
target:
description: "Specified target toolchain, ex. x86_64-unknown-linux-gnu"
type: string
required: true
options:
- x86_64-unknown-linux-gnu
- aarch64-unknown-linux-gnu
- x86_64-apple-darwin
- aarch64-apple-darwin
- aarch64-unknown-linux-musl
- x86_64-unknown-linux-musl
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used as a cache key, so could be anything. This parameter and options looks like copied from rust toolchain selector.
I think you can remove this parameter and use combination of "os/runner + version" as a cache key

run: |
cd ~/valkey
sudo make install
echo 'export PATH=/usr/local/bin:$PATH' >>~/.bash_profile
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this? AFAIK make install copies binaries to /usr/bin, so they become available in $PATH

Comment on lines 83 to 89
uses: ./.github/workflows/install-redis
with:
redis-version: ${{ matrix.engine.version }}

- name: Install Valkey
if: ${{ matrix.engine.type == 'valkey' }}
uses: ./.github/workflows/install-valkey
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need to combine these steps into install-engine or install-db?

@@ -11,6 +11,7 @@ on:
- .github/workflows/node.yml
- .github/workflows/build-node-wrapper/action.yml
- .github/workflows/install-shared-dependencies/action.yml
- .github/workflows/install-valkey/action.yml
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add to other CI workflows please (java, C#, go - all)

@avifenesh
Copy link
Collaborator

As for previews version of 7.2.5 valkey and redis are the same, and from there we want to test just against valkey ATM, so why not just change all to valkey and thats it?
And if we want to keep test against redis i'll change it to test against valkey till 7.2.5, and from there add redis tests, and not the opposite.

@barshaul barshaul force-pushed the valkey_ci branch 4 times, most recently from 3f80f31 to 1c75943 Compare June 27, 2024 17:10
@barshaul barshaul changed the title CI: Add Support for Valkey 7.2.5 CI: Add Support for Valkey 6.2.14, 7.0.15, 7.2.4 and 7.2.5 Jun 27, 2024
@barshaul barshaul changed the title CI: Add Support for Valkey 6.2.14, 7.0.15, 7.2.4 and 7.2.5 CI: Add Support for Valkey 6.2, 7.0 and 7.2 Jun 29, 2024
@barshaul
Copy link
Collaborator Author

Moved to #1711

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/CD CI/CD related
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants