-
Notifications
You must be signed in to change notification settings - Fork 14
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
Postres secrets #310
base: develop
Are you sure you want to change the base?
Postres secrets #310
Conversation
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.
By removing the .env.docker
you remove the default .env
used in the docker-compose.yml
and you force all the developers to make an extra step of configuration by creation the .env
from the .env.example
.
- I'm not sure that it's interesting since every additional step in a quick-start is always a source of potential problem for newcomers
- If we do this change, we should at least explain it in the documentation since it's not just in case we want to
modify it
anymore
Other than that, putting docker config from ENV is a nice improvement I think.
@@ -31,4 +31,4 @@ services: | |||
volumes: | |||
- ./:/code | |||
ports: | |||
- "8001:8001" | |||
- "8010:8010" |
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.
Why do you change the port ? I don't understand this change.
And if it's correct to change it, we should also update the documentation
API-Volontaria/docs/getting_started/docker.md
Lines 49 to 58 in 9bd7c63
## Using the services | |
You can now visit these links to validate the installation: | |
- The root of the API (will need authentication as a first step): [http://localhost:8000/](http://localhost:8000/) | |
- The admin site: [http://localhost:8000/admin/](http://localhost:8000/admin/) | |
- The documentation you're reading: [http://localhost:8001/](http://localhost:8001/) | |
Do not hesitate to check the API section of this documentation to know how to authenticate yourself with the API | |
and begin asking queries. |
def pg_isready(host, user, password, dbname): | ||
while time() - start_time < check_timeout: | ||
try: | ||
conn = psycopg2.connect(**vars()) | ||
conn = psycopg2.connect(**config) | ||
logger.info("Postgres is ready! ✨ 💅") | ||
conn.close() | ||
return True | ||
except psycopg2.OperationalError: | ||
logger.info( | ||
f"Postgres isn't ready. Waiting for {check_interval} " | ||
f"{interval_unit}..." | ||
f"{interval_unit}... using {config}" | ||
) | ||
sleep(check_interval) | ||
|
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.
Why not using the vars()
? It was a good way to make it configurable.
If we don't use the vars anymore, we should remove it from the function parameters and change the call on line 42
What have you changed ?
...
improve the way postgres secrets are managed
How did you change it ?
all postgres secrets like db name host, user and pass will use .env variables everywhere preventing hard code this values.
...
Is there a special consideration?
...
Verification :
CONTRIBUTING.md
CONTRIBUTING.md