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

[Draft] [CAE-580] Migrate Lettuce test setup to use client-lib-test #3158

Draft
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

kiryazovi-redis
Copy link
Collaborator

@kiryazovi-redis kiryazovi-redis commented Feb 3, 2025

Make sure that:

  • You have read the contribution guidelines.
  • You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
  • You applied code formatting rules using the mvn formatter:format target. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

Failing tests:
1 Keystore test.

That is the reason why this review is draft.

After the tests pass, will likely reduce redis.conf files in size as some of the config are provided by default.
Also, there is some small work left on the docker-compose.

@kiryazovi-redis kiryazovi-redis force-pushed the topic/kiryazovi-redis/use-docker-for-tests branch from 24693f5 to b9dceb8 Compare February 6, 2025 15:12
@kiryazovi-redis kiryazovi-redis force-pushed the topic/kiryazovi-redis/use-docker-for-tests branch from 1f6b443 to c4e3b08 Compare February 7, 2025 18:31
@kiryazovi-redis kiryazovi-redis force-pushed the topic/kiryazovi-redis/use-docker-for-tests branch from e79ac3f to 7892951 Compare February 10, 2025 10:09
Copy link
Collaborator Author

@kiryazovi-redis kiryazovi-redis left a comment

Choose a reason for hiding this comment

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

Self review, so our sync-up today goes faster. (after discussion with Ivo)

Makefile Show resolved Hide resolved
@@ -66,11 +69,13 @@ class SslIntegrationTests extends TestSupport {

private static final String KEYSTORE = "work/keystore.jks";

private static final String TRUSTSTORE = "work/truststore.jks";
private static File truststoreFile0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discuss today with Tisho if we want to leave this in that format, as it will mean we deprecate makefile entirely.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Remove old way of deploying.

@@ -66,11 +69,13 @@ class SslIntegrationTests extends TestSupport {

private static final String KEYSTORE = "work/keystore.jks";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ivo is helping me with this testcase, as its not passing.


private static final String TRUST_STORE_TYPE = "PKCS12";

private static final String TEST_WORK_FOLDER = System.getenv().getOrDefault("TEST_WORK_FOLDER", "/tmp/redis-env-work");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discuss with TIsho if we need to change this.

@@ -0,0 +1 @@
REDIS_ENV_WORK_DIR=/tmp/redis-env-work
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently working on to see if we want versions of Redis in 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.

Create multiple env files and close this convo.

@@ -0,0 +1,10 @@
port 6478
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thin out the redis.conf or do that in a future commit.

@@ -0,0 +1,123 @@
services:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have to integrate this into the nightlies.

networks:
- redis-network

redis-standalone-sentinel-controlled:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps tech debt question. Do we want the json tests added into this one?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lets make this into another task, but try to go to this direction.

We want to remove testcontainers, and get the dockercompose to be in one place.

Then in the future we'll add back testcontainers.

services:
# Standalone Redis Servers
redis-standalone-0:
image: redislabs/client-libs-test:8.0-M02
Copy link
Contributor

Choose a reason for hiding this comment

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

We have recently introduced the option for test matrix testing against different versions.
(7.2, 7.4, 8.0 )
We should make the test container version configurable
Use scenarios that need to be covered:

  1. During PR integration test - we should be able to test against concrete version
  2. Local dev - we should be able to bootstrap a test env from specified version

@@ -111,9 +116,24 @@ class SslIntegrationTests extends TestSupport {

@BeforeAll
static void beforeClass() {
Path path0 = createAndSaveTestTruststore("redis-standalone-0", Paths.get("redis-standalone-0/work/tls"), "changeit");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tishun : make sure to fix the work directory, try to put in in target
Ivo: when docker compose to go in the target + test to go and find it in the correct target folder.

Makefile Outdated Show resolved Hide resolved
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.

3 participants