Django upgrade 21: reset migrations

eMBee commented 1 month ago

follows #128

marcoag commented 1 month ago

@eMBee please take a look at the conflicts.

marcoag commented 1 month ago

Thanks for the update @eMBee. This one deserves a proper check with a db from scratch, i'll try to find some time for that.

marcoag commented 1 month ago

I did a run with a new db and I cant login. It seems that this migrations also introduce a new error:

eventyay-tickets  | Running migrations:
eventyay-tickets  |   Applying contenttypes.0001_initial... OK
db                | 2024-05-29 21:52:53.114 UTC [64] ERROR:  relation "django_content_type" already exists
db                | 2024-05-29 21:52:53.114 UTC [64] STATEMENT:  CREATE TABLE "django_content_type" ("id" integer NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "name" varchar(100) NOT NULL, "app_label" varchar(100) NOT NULL, "model" varchar(100) NOT NULL)
eventyay-tickets  | Traceback (most recent call last):
eventyay-tickets  |   File "/usr/local/lib/python3.8/site-packages/django/db/backends/utils.py", line 87, in _execute
eventyay-tickets  |     return self.cursor.execute(sql)
eventyay-tickets  | psycopg2.errors.DuplicateTable: relation "django_content_type" already exists

I posted the log at: https://gist.github.com/marcoag/a741da390313c7a76402adc9c044c14c

You can compare with a log from a deploymet from the development head:

https://gist.github.com/marcoag/c3f96f9ff6e6648c0afcd1ea099a3961

I also cannot login, the default admin user/pass does not work.

marcoag commented 1 month ago

I also realized deploying development head in a clean HEAD gives me the following error when trying to login:

eventyay-tickets  | django.db.utils.ProgrammingError: column pretixbase_event.sales_channels does not exist
eventyay-tickets  | LINE 1: ...subevents", "pretixbase_event"."seating_plan_id", "pretixbas...

I'm wondering if we introduced some error with the previous PR. Any hing on that pretixbase_event.sales_channels @eMBee ?

hongquan commented 1 month ago

@marcoag You are testing in containers. Could you give me your docker-compose file so I can try reproducing? This log said that the Django container failed to connect to DB container and that's why the DB migration failed.

eventyay-tickets  |   File "/usr/local/lib/python3.8/site-packages/django/db/backends/postgresql/base.py", line 275, in get_new_connection
eventyay-tickets  |     connection = self.Database.connect(**conn_params)
eventyay-tickets  |   File "/usr/local/lib/python3.8/site-packages/psycopg2/__init__.py", line 122, in connect
eventyay-tickets  |     conn = _connect(dsn, connection_factory=connection_factory, **kwasync)
eventyay-tickets  | django.db.utils.OperationalError: connection to server at "db" (172.18.0.3), port 5432 failed: Connection refused
eventyay-tickets  |     Is the server running on that host and accepting TCP/IP connections?
eventyay-tickets  |

mariobehling commented 1 month ago

I think this is the docker compose that is used: https://github.com/fossasia/eventyay-docker/blob/main/docker-compose.yml

hongquan commented 1 month ago

The docker-compose file in https://github.com/fossasia/eventyay-docker/blob/main/docker-compose.yml can not be used to test this PR, because:

It doesn't contain the source code of eMBee:

pretix:
  image: eventyay/eventyay-tickets:development
  container_name: eventyay-tickets

Its DB config is wrong (use Postgres but mount MySQL directory):

db:
 image: postgis/postgis:12-2.5-alpine
 container_name: pretalx-db
 restart: unless-stopped
 volumes:
   - pretalx-db:/var/lib/mysql

hongquan commented 1 month ago

The most trustworthy environment to test is non-Docker environment, because it doesn't add up external factors which are potential source of bugs (if we test on Docker, we add source of bugs like the Dockerfile, docker-compose, which is not related to the issue that this PR is trying to solve).

marcoag commented 4 weeks ago

@marcoag You are testing in containers. Could you give me your docker-compose file so I can try reproducing? This log said that the Django container failed to connect to DB container and that's why the DB migration failed.

eventyay-tickets  |   File "/usr/local/lib/python3.8/site-packages/django/db/backends/postgresql/base.py", line 275, in get_new_connection
eventyay-tickets  |     connection = self.Database.connect(**conn_params)
eventyay-tickets  |   File "/usr/local/lib/python3.8/site-packages/psycopg2/__init__.py", line 122, in connect
eventyay-tickets  |     conn = _connect(dsn, connection_factory=connection_factory, **kwasync)
eventyay-tickets  | django.db.utils.OperationalError: connection to server at "db" (172.18.0.3), port 5432 failed: Connection refused
eventyay-tickets  |     Is the server running on that host and accepting TCP/IP connections?
eventyay-tickets  |

I belive that is a race condition, there's an intent of connection on line but the db is not ready until line 317, you can see later that migrations start to apply and they work but some fail:

eventyay-tickets  | Running migrations:
eventyay-tickets  |   Applying contenttypes.0001_initial... OK
db                | 2024-05-29 21:52:53.114 UTC [64] ERROR:  relation "django_content_type" already exists
db                | 2024-05-29 21:52:53.114 UTC [64] STATEMENT:  CREATE TABLE "django_content_type" ("id" integer NOT NULL PRIMARY KEY GENERATED BY DEFAULT AS IDENTITY, "name" varchar(100) NOT NULL, "app_label" varchar(100) NOT NULL, "model" varchar(100) NOT NULL)
eventyay-tickets  | Traceback (most recent call last):
eventyay-tickets  |   File "/usr/local/lib/python3.8/site-packages/django/db/backends/utils.py", line 87, in _execute
eventyay-tickets  |     return self.cursor.execute(sql)
eventyay-tickets  | psycopg2.errors.DuplicateTable: relation "django_content_type" already exists

which is clearly not an error of non connection but an error with the migration itself.

marcoag commented 4 weeks ago

The docker-compose file in https://github.com/fossasia/eventyay-docker/blob/main/docker-compose.yml can not be used to test this PR, because:
* It doesn't contain the source code of eMBee:
  ```yaml
   pretix:
     image: eventyay/eventyay-tickets:development
     container_name: eventyay-tickets
  ```

The docker-compose uses some image from docker hub. It can't have the code from embee magically written there because you need to build an image first and give it a name, you can build it yourself and modify that image field to use the image you built.

* Its DB config is wrong (use Postgres but mount MySQL directory):
  ```yaml
  db:
    image: postgis/postgis:12-2.5-alpine
    container_name: pretalx-db
    restart: unless-stopped
    volumes:
      - pretalx-db:/var/lib/mysql
  ```

This is only mounting a volume in a certain location called mysql, I blieve it should not be relevant to the case we are discussing here.

marcoag commented 4 weeks ago

The most trustworthy environment to test is non-Docker environment, because it doesn't add up external factors which are potential source of bugs (if we test on Docker, we add source of bugs like the Dockerfile, docker-compose, which is not related to the issue that this PR is trying to solve).

I'm not gonna go down this discussion since it's unrelated to this PR, but we run things on docker and we want developers to test on docker. I encourage you to read about the benefits of testing inside docker.

hongquan commented 4 weeks ago

I'm not gonna go down this discussion since it's unrelated to this PR, but we run things on docker and we want developers to test on docker. I encourage you to read about the benefits of testing inside docker.

I don't object using Docker container. I mean we need to divide things to identify bugs and solve separately:

1) Base code (without container) 2) Code + container

Bugs related to Dockerfile, docker-compose need to be fixed in other PRs or repos. If we mix these two, it is difficult (and wasting time) to identify bugs, it even worse if MBee code is good but because of bugs in container setup that we blame his work and stuck in a loop.

I blieve it should not be relevant to the case we are discussing here.

This is a very good example why we should not mix two things here. Discussion about eMBee Django code suddenly involves container stuff, which you are agree that it is not relevant.

So, to make this PR progress, I suggest to test this PR without containers. The test with containers should be done in another work which can be titled: "Docker container integation".

hongquan commented 3 weeks ago

No more warning after adding DEFAULT_AUTO_FIELD settings:

fossasia / eventyay-tickets

Django upgrade 21: reset migrations #129