Skip to content

Conversation

@mircorudolph
Copy link
Contributor

No description provided.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +235 to +239
# Fixed commands (check for user)
# give hint on actual docker name container E.g. bold_buck random name
docker exec -i bold_buck psql -U citycatalyst -d postgres -v ON_ERROR_STOP=1 -c "CREATE USER climateadvisor WITH PASSWORD 'climateadvisor';"

docker exec -i bold_buck psql -U citycatalyst -d postgres -v ON_ERROR_STOP=1 -c "CREATE DATABASE climateadvisor OWNER climateadvisor;"

Choose a reason for hiding this comment

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

P1 Badge Use created container name in docker exec examples

The setup instructions start the PostgreSQL container with --name ca-postgres, but the subsequent docker exec commands target bold_buck. Unless the user happens to have a container with that randomly generated name, these commands fail with “No such container: bold_buck”, preventing database initialization. The examples should reference ca-postgres (or explain how to discover the actual container name) so the instructions can be executed as written.

Useful? React with 👍 / 👎.

Comment on lines +246 to 250
docker exec -i ca-postgres psql -U postgres -d postgres
CREATE USER climateadvisor WITH PASSWORD 'climateadvisor';
CREATE DATABASE climateadvisor OWNER climateadvisor;
GRANT ALL PRIVILEGES ON DATABASE climateadvisor TO climateadvisor;
ALTER USER climateadvisor CREATEDB;

Choose a reason for hiding this comment

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

P1 Badge SQL commands no longer piped into psql

Previously the README used a here-document to feed the SQL statements into psql. The updated snippet runs docker exec -i ca-postgres psql -U postgres -d postgres and then lists the SQL lines as separate shell commands. Executing the instructions now causes the shell to try running CREATE USER … and the other SQL as binaries, resulting in command not found and leaving the database user and schema uncreated. The SQL needs to be passed via -c arguments or wrapped again in a here-document so it executes inside psql.

Useful? React with 👍 / 👎.

@mircorudolph
Copy link
Contributor Author

Needed to also change in the .envs: CC_BASE_URL=http://localhost:3000 (from internal.docker....)

Copy link
Contributor

@lemilonkh lemilonkh left a comment

Choose a reason for hiding this comment

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

Looks good, thanks 👍

uvicorn app.main:app --host 0.0.0.0 --port 8080 --reload
```

TODO: instead of uvicorn chanfe config to use docker container here
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be fixed in this PR?

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.

4 participants