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.

Sign up for GitHub

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

Jump to bottom

Make PostgreSQL and Redis connection strings configurable #627

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Make PostgreSQL and Redis connection strings configurable #627

wants to merge 8 commits into from
+14 −2

Conversation

imanimen
Copy link

@imanimen imanimen commented Oct 15, 2024 •
edited by mostafa
Loading

Ticket(s)

Closes #563

Description

Configured the docker-compose.yaml, gatewayd_plugins.yaml to read from the the Environment variables for redis and postresql url string

Related PRs

N/A

Development Checklist

  • I have added a descriptive title to this PR.
  • I have squashed related commits together.
  • I have rebased my branch on top of the latest main branch.
  • I have performed a self-review of my own code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have added docstring(s) to my code.
  • I have made corresponding changes to the documentation (docs).
  • I have updated docs using make gen-docs command.
  • I have added tests for my changes.
  • I have signed all the commits.

Legal Checklist

@github-actions GitHub Actions

This comment was marked as resolved.

Sign in to view
mostafa

This comment was marked as resolved.

Sign in to view

@mostafa mostafa changed the title Iman dev/issues #563 Make PostgreSQL and Redis connection strings configurable Oct 20, 2024
mostafa
mostafa requested changes Oct 20, 2024
View reviewed changes
Copy link
Member

@mostafa mostafa left a comment

Choose a reason for hiding this comment

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

IMHO, the original ticket, #563, seems to be stale now and the only change that can benefit Docker users is to change the Redis URL.

I can change them myself if you want.

docker-compose.yaml
@@ -65,6 +75,8 @@ services:
# https://docs.gatewayd.io/using-gatewayd/configuration#environment-variables
- GATEWAYD_CLIENTS_DEFAULT_WRITES_ADDRESS=write-postgres:5432
- GATEWAYD_CLIENTS_DEFAULT_READS_ADDRESS=read-postgres:5432
# Use the connection string set in install_plugins
- POSTGRES_URL=${POSTGRES_URL}
Copy link
Member

Choose a reason for hiding this comment

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

This is not used anywhere in the setup.sh script or config files and seems unnecessary.

docker-compose.yaml
Comment on lines +22 to +26
# Allow custom PostgreSQL connection string
- POSTGRES_USER=postgres
- POSTGRES_PASSWORD=postgres
- POSTGRES_DB=postgres
- POSTGRES_URL=postgresql://${POSTGRES_USER}:${POSTGRES_PASSWORD}@write-postgres:5432/${POSTGRES_DB}
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary. Explained why below.

gatewayd_plugins.yaml
@@ -72,7 +72,7 @@ plugins:
env:
- MAGIC_COOKIE_KEY=GATEWAYD_PLUGIN
- MAGIC_COOKIE_VALUE=5712b87aa5d7e9f9e9ab643e6603181c5b796015cb1c09d6f5ada882bf2a1872
- REDIS_URL=redis://localhost:6379/0
- REDIS_URL=${REDIS_URL} # Use environment variable for Redis URL
Copy link
Member

Choose a reason for hiding this comment

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

Changing these will make this file unusable if someone tries to use it outside Docker and docker-compose. Hence, you should change the setup.sh script to update this instead of changing these files directly. Also, the PostgreSQL addresses and ports are already configurable via environment variables, hence you don't need to change that.

gatewayd_plugins.yaml
@@ -43,7 +43,7 @@ actionRedis:
enabled: false

# if enabled, the url and channel are used to connect to the Redis server.
address: localhost:6379
address: ${REDIS_URL} # Use environment variable for Redis URL
Copy link
Member

Choose a reason for hiding this comment

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

Change this to the following, so that it gets replaced by the value of the REDIS_URL environment variable, defined here:

Suggested change
address: ${REDIS_URL} # Use environment variable for Redis URL
address: redis://localhost:6379/0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mostafa mostafa mostafa requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make the Postgres and Redis connection strings customizable in the docker-compose
2 participants
@imanimen @mostafa