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

Do not rely on Redis compiled from source code #3734

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gerzse
Copy link
Contributor

@gerzse gerzse commented Feb 20, 2024

Issue #3710

Instead of building Redis server from source code, use Docker images. Prepare docker-compose stacks that accept the version of the Docker image as parameter, so we can run against multiple versions in CI.

The purpose of this change is to strictly move everything to Docker, without trying to change anything else. Some Java tests had to be adapted, still, because now the Redis instances are accessed via localhost, while internally in the docker-compose network they see each other with other IPs and ports.

Polish a bit the Makefile, for example to run exactly the same thing that runs in CI, so when we run the tests locally we get exactly the same coverage.

.gitignore Outdated Show resolved Hide resolved
Copy link
Contributor

@chayim chayim left a comment

Choose a reason for hiding this comment

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

Top-line comment:

  1. Is there some way to remove all the hardcoded ports (or another PR?). It seems to be a constant theme throughout, and worth the debt removal.
  2. That raises the question, is there a (painless) way to inject into the various configs, like stunnel and the dockers.

@@ -216,14 +216,14 @@ public void testMigrate() {
node2.set("e", "e");
} catch (JedisMovedDataException jme) {
assertEquals(15363, jme.getSlot());
assertEquals(new HostAndPort(LOCAL_IP, nodeInfo3.getPort()), jme.getTargetNode());
assertEquals(new HostAndPort(JedisClusterTestUtil.getClusterIp(3), 6379), jme.getTargetNode());
Copy link
Contributor

Choose a reason for hiding this comment

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

Same general comment re ports - throughout.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we have to do something about this. It's so spaghetti right now, implied relations between ports and stuff. My intention was to leave that for another PR, go step by step. Here I did a minimal mapping to the new Docker IPs and ports.

@@ -0,0 +1,48 @@
-----BEGIN RSA PRIVATE KEY-----
Copy link
Contributor

Choose a reason for hiding this comment

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

So we made this exact same mistake in redis-py, which IMHO is tech debt. When I originally committed it, I thought it smarter to commit junk certs (and a README), but it led to fallout. Since they're generated via this script from redis, maybe we should just fetch it, and run it first? We'd need a modification given the last line, but the only requirement is openssl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, let's integrate that. In another PR?

@gerzse gerzse force-pushed the dont-compile-redis-from-source branch from cf00093 to e5776d0 Compare February 21, 2024 09:15
@gerzse gerzse requested a review from sazzad16 February 21, 2024 09:16
Issue redis#3710

Instead of building Redis server from source code, use Docker images.
Prepare docker-compose stacks that accept the version of the Docker
image as parameter, so we can run against multiple versions in CI.

The purpose of this change is to strictly move everything to Docker,
without trying to change anything else. Some Java tests had to be
adapted, still, because now the Redis instances are accessed via
localhost, while internally in the docker-compose network they see each
other with other IPs and ports.

Polish a bit the Makefile, for example to run exactly the same thing
that runs in CI, so when we run the tests locally we get exactly the
same coverage.
@gerzse gerzse force-pushed the dont-compile-redis-from-source branch from e5776d0 to 9286d0c Compare February 21, 2024 09:19
Don't change some imports so much.

Fix a typo, that was me trying to open pavucontrol.
Fix some failing tests, the mapping of hosts/ports was broken by the
earlier changes.

Move some core around, as suggested during code review.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants