search

ovh / celery-director

Simple and rapid framework to build workflows with Celery
https://ovh.github.io/celery-director/
BSD 3-Clause "New" or "Revised" License
534 stars 58 forks source link

feat: DB table name prefixing using DB_TABLE_PREFIX env var (+docs: corrected typos) #119

Closed fmalina closed 2 years ago

fmalina commented 2 years ago

Signed-off-by: Frantisek Malina fmalina@pm.me

CodePint commented 2 years ago

@ncrocfer are you able to merge this?

CodePint commented 2 years ago

Hi @fmalina,

First of all thank you for this PR and sorry for the delay to review and merge this PR.

Unfortunately we can not approve it for the following reasons:

  1. even if you change settings.py to handle a DIRECTOR_DATABASE_TABLE_PREFIX variable you don't use it in the rest of the code. Instead you use an environment variable (os.getenv("DIRECTOR_DATABASE_TABLE_PREFIX", "")) to prefix the tables. You have to use app.config["DATABASE_TABLE_PREFIX"], as the rest of the code does for each configuration property.

So the feature is not working as expected:

$ cat .env
...
DIRECTOR_DATABASE_URI="postgresql://localhost:5432/director_pr_119"
DIRECTOR_DATABASE_TABLE_PREFIX="foobar"
...
$ director db upgrade
$ psql -h localhost -d director_pr_119
director_pr_119=# \dt
              List of relations
 Schema |      Name       | Type  |  Owner
--------+-----------------+-------+----------
 public | alembic_version | table | ncrocfer
 public | tasks           | table | ncrocfer
 public | users           | table | ncrocfer
 public | workflows       | table | ncrocfer
  1. The DIRECTOR_DATABASE_TABLE_PREFIX is not added in the default configuration file (inside commands/init.py file)
  2. The tasks, users and workflows tables will be changed, but what about the alembic_version table?
  3. You have too many commits for 2 features, the best is to provide 2 commits using the conventional commit format (https://www.conventionalcommits.org/en/v1.0.0-beta.4/)
  4. If you can add some tests it will be a great improvement of this PR

When you will be able to fix all these comments we'll be able to review & merge your PR. Thanks again for your contribution.

@fmalina You are welcome to make these changes, but aware you are probably busy with other things mate.

@ncrocfer If not i'll have a look and see If I can pick this up as its something we need internally.

fmalina commented 2 years ago

@CodePint I'd welcome you to pick this up yourself, I am fully aware of all these shortcomings myself and undone my changes to fully implement this as I struggled with getting to the app.config in places so stuck to env var

CodePint commented 2 years ago

@CodePint I'd welcome you to pick this up yourself, I am fully aware of all these shortcomings myself and undone my changes to fully implement this as I struggled with getting to the app.config in places so stuck to env var

Heya @fmalina thanks for getting back to me, much appreciated Yeah I do remember you mentioning there were some outstanding issues here, I will pick this up and try to finish it off. :)

ncrocfer commented 2 years ago

@fmalina @CodePint I clean the backlog and close this PR, of course you will be able to open another one when you'll be ready.