-
Notifications
You must be signed in to change notification settings - Fork 44.3k
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
fix docker-compose setup #1198
fix docker-compose setup #1198
Conversation
@@ -14,3 +14,4 @@ services: | |||
|
|||
redis: | |||
image: "redis/redis-stack-server:latest" | |||
container_name: "redis-stack-server" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this is necessary? What is wrong with the default name of redis
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikekelly I have same question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing wrong with it, I could've used redis but in my local testing the image doesn't spin up as redis it's like auto-gpt-redis-1. I could change this to redis but I think specifying a name is useful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so that there's something consistent people can put in their .env for the host
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoop my mistake, no need to specify the name. I at least think the readme update is useful and can modify the readme or close the pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A specific name instead of a generic name helps avoid conflicts and/or confusion when multiple Redis containers may be running on the same host.
As a best practice, it's better to be specific than generic, especially when you can have OSS Redis, the standard engine; Redis Stack extension with builtin RedisInsight for Dev; and Redis Stack Server, the extension without RedisInsight for Production.
As convention, using the origin product name plus optional specific identifiers helps any new comers and avoids misunderstandings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aosan if you decide to go in that direction, you're still better off achieving this through the name of the service. Docker compose provides a network where the containers can address each other via their service name.
2ed5000
to
b4186f4
Compare
README.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not related to docker-compose. Revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops auto formatted the readme in the rebase
b4186f4
to
bbe7f5d
Compare
@@ -14,3 +14,4 @@ services: | |||
|
|||
redis: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This service can be renamed to redis-stack-server and then there's is no need to specify the container_name
Do we have consensus agreement from the discussion? Hard to see for me... |
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
The change to |
Background
Earlier today #836 was merged in but I think it needed an update to the readme for explanation and a tweak to the
docker-compose.yml
for clarity.Changes
Added
container_name: "redis-stack-server"
on the redis container so it's easy to drop into the env.Updated the readme with steps for running the docker-compose + redis.
Documentation
Updated the readme.
Test Plan
Have tested locally - this is the failure before changes -- LIGHT MODE WARNING
PR Quality Checklist