Skip to content

Conversation

@hzloc
Copy link

@hzloc hzloc commented Aug 16, 2024

  • docker setup

Copy link
Collaborator

@mbanck-cd mbanck-cd left a comment

Choose a reason for hiding this comment

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

Hi,

thanks for the Docker setup! I finally got around to review it and have a few comments

#!/bin/sh
set -eux

psql -v ON_ERROR_STOP=1 --username "$POSTGRES_USER" --dbname "$POSTGRES_DB" <<-EOSQL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is $POSTGRES_DB coming from, I don't see it set anywhere else?

environment:
POSTGRES_PASSWORD: omdbmovies
POSTGRES_USER: omdbmovies
POSTGRES_NAME: omdbmovies
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was that supposed to be POSTGRES_DB (see below)?

POSTGRES_USER: omdbmovies
POSTGRES_NAME: omdbmovies
ports:
- "54325:5432" No newline at end of file
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that 54325 port intentional? Wouldn't that need additional documentation on how to reach Postgres inside the container?

Comment on lines +6 to +7
CREATE DATABASE demo;
GRANT ALL PRIVILEGES ON DATABASE demo TO docker;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This demo database does not seem to get used anywhere?

Probably creating and using a omdb database would be better?

@@ -0,0 +1,11 @@
FROM postgres:14.5
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know it's been a while, but the Postgres version should be bumped to something newer (or latest?)

FROM postgres:14.5
# requirements to run ./download
RUN apt-get update
RUN apt-get install -y wget && apt-get install -y bzip2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Those could be folded into one apt-get call

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.

2 participants