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

Update Valkey version to 8.0.0 in test matrix and version checks #2296

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

shohamazon
Copy link
Collaborator

@shohamazon shohamazon commented Sep 16, 2024

instead of testing with valkey8 rc on our ci, we will now test with valkey 8
inn valkey 8 rc the valkey version was 7.9.240, so all the version checks needs to be updated

@shohamazon shohamazon self-assigned this Sep 16, 2024
@shohamazon shohamazon requested a review from a team as a code owner September 16, 2024 14:10
@shohamazon shohamazon added the Valkey-8 Support for Valkey-8 features label Sep 16, 2024
@shohamazon shohamazon linked an issue Sep 16, 2024 that may be closed by this pull request
expect(await client.getrange(key, -200, -100)).toEqual(
cluster.checkIfServerVersionLessThan("8.0.0") ? "T" : "",
);
expect(await client.getrange(key, -200, -100)).toEqual("T");
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

assert await glide_client.getrange(key, -200, -100) == value[0].encode()
else:
assert await glide_client.getrange(key, -200, -100) == b""
assert await glide_client.getrange(key, -200, -100) == value[0].encode()
Copy link
Collaborator

Choose a reason for hiding this comment

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

and here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

same

@ikolomi
Copy link
Collaborator

ikolomi commented Sep 16, 2024

@shohamazon you should push to release branch

}
} catch (Exception e) {
// General catch block in case any other exception occurs during client cleanup
System.err.println("Error during cleanup for client: " + e.getMessage());
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand Sep 17, 2024

Choose a reason for hiding this comment

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

Don't suppress an exception - we won't see a failure if it occurs (no one reads logs if CI is green). Junit catches them and reports.
Please remove try-catch blocks, we need to properly unsubscribe to avoid NOSUB error from valkey 8

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am waiting for your pr actually, I added this without noticing you added a fix
Can you also add a fix to node?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will do. Why it doesn't fail on python? Something it missing there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we check what commands we ran so it knows what exactly to unsubscribe from, but I need to take a look again at the test teardown

Signed-off-by: Shoham Elias <shohame@amazon.com>
@ikolomi
Copy link
Collaborator

ikolomi commented Sep 19, 2024

@shohamazon please link the originating issue

@shohamazon shohamazon linked an issue Sep 19, 2024 that may be closed by this pull request
7 tasks
@shohamazon shohamazon merged commit 1c334fd into valkey-io:release-1.1 Sep 19, 2024
23 checks passed
@shohamazon shohamazon deleted the valkey-8 branch September 19, 2024 10:07
shohamazon added a commit to shohamazon/glide-for-redis that referenced this pull request Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release blocker Valkey-8 Support for Valkey-8 features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check Valkey-8 version correctly Valkey 8 support
3 participants