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

Load Redis test endpoints from config file or env vars #294

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

Conversation

uglide
Copy link
Contributor

@uglide uglide commented Apr 30, 2024

No description provided.

@uglide uglide changed the title [WIP] Load Redis test endpoints from config file or env vars Load Redis test endpoints from config file or env vars Apr 30, 2024
@uglide uglide marked this pull request as ready for review April 30, 2024 13:04

redisEndpoints = parsedEndpoints ?? throw new Exception("Failed to parse the Redis endpoints configuration.");
}
else
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an idea, would it work to have a default JSON file to load, so we don't do any config from code?

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 can replace it with a default JSON file. I did it only to keep it backward compatible with the existing GitHub workflows.

configurationOptions.Password = password;
}

// TODO(imalinovskiy): Add support for TLS
Copy link

Choose a reason for hiding this comment

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

Can we perhaps list somewhere the environments we would like to have?

I think the existing tests (which make sense to us) are a good reference as to what we need.

Everybody can suggest additions / deletions to this list

Copy link
Contributor Author

@uglide uglide May 9, 2024

Choose a reason for hiding this comment

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

The list of required environments depends on the current test coverage. For example, in Jedis, we need a huge list of Redis endpoints https://github.com/redis/jedis/pull/3836/files#diff-c731b96d111ab92ab2f88dcbe2e36abef53c269a71f96598463e6cc3b70b7acd and it doesn't yet cover the Sentinel and Cluster endpoints. As a first stage, I suggest identifying those endpoints and collecting them in endpoints.json file in each repo.

Copy link

Choose a reason for hiding this comment

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

Yes, this is what I meant, gather a list so we know what we need. Thanks!

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.

None yet

3 participants