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

Java: new README.md #395

Merged
merged 14 commits into from
Jul 2, 2024
Merged

Conversation

cyip10
Copy link

@cyip10 cyip10 commented Jun 27, 2024

Had an issue with git. Had to duplicate the PR to this one. Will close the old one.

#388

java/README.md Outdated Show resolved Hide resolved
@Yury-Fridlyand Yury-Fridlyand mentioned this pull request Jun 27, 2024
java/README.md Outdated Show resolved Hide resolved
java/README.md Outdated Show resolved Hide resolved
java/README.md Outdated Show resolved Hide resolved
java/README.md Outdated Show resolved Hide resolved
java/README.md Outdated Show resolved Hide resolved
java/README.md Outdated Show resolved Hide resolved
java/README.md Outdated Show resolved Hide resolved
java/README.md Outdated Show resolved Hide resolved
java/README.md Outdated Show resolved Hide resolved
java/README.md Outdated Show resolved Hide resolved
@cyip10 cyip10 force-pushed the java/dev_cyip10_readmeNew branch from 4a3924c to 54a16ea Compare June 28, 2024 16:54
@GumpacG GumpacG changed the base branch from java/integ_cyip10_readme to main June 28, 2024 17:18
@GumpacG GumpacG changed the base branch from main to java/integ_cyip10_readme June 28, 2024 17:18
@GumpacG GumpacG changed the base branch from java/integ_cyip10_readme to main June 28, 2024 17:30
@GumpacG GumpacG changed the base branch from main to java/integ_cyip10_readme June 28, 2024 17:30
java/README.md Outdated Show resolved Hide resolved
java/README.md Outdated Show resolved Hide resolved
java/README.md Outdated Show resolved Hide resolved
java/README.md Outdated Show resolved Hide resolved
Copy link

@GumpacG GumpacG left a comment

Choose a reason for hiding this comment

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

LGTM! Assuming comments above are addressed.

java/README.md Outdated Show resolved Hide resolved
java/README.md Outdated Show resolved Hide resolved
java/README.md Show resolved Hide resolved
java/README.md Outdated Show resolved Hide resolved
java/README.md Outdated Show resolved Hide resolved
### Build from source

Software Dependencies:
#### Prerequisites
Copy link

Choose a reason for hiding this comment

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

We should mention that an existing Valkey-compatible server is required to run the integration tests. Probably shouldn't say "install redis".

Choose a reason for hiding this comment

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

I believe IT expects redis-server to be installed.
https://github.com/aws/glide-for-redis/blob/main/utils/cluster_manager.py#L287

Copy link

Choose a reason for hiding this comment

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

Wait, should we leave redis references in the code?

Choose a reason for hiding this comment

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

until there's an equivalent setup for valkey-server, we don't have a choice.

Choose a reason for hiding this comment

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

java/README.md Show resolved Hide resolved
java/README.md Outdated Show resolved Hide resolved
java/README.md Outdated Show resolved Hide resolved
Copy link

@alexey-temnikov alexey-temnikov left a comment

Choose a reason for hiding this comment

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

Add a reference to the Java Readme from the root Readme file (similar as done for Python and Node.JS)

Add references in the root README.md under -
Getting Started

@cyip10 cyip10 merged commit 66b99f2 into java/integ_cyip10_readme Jul 2, 2024
11 checks passed
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.

9 participants