Skip to content
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

MDBF-423: Change default connection to database to use host different than dock… #3

Closed
wants to merge 1 commit into from

Conversation

an3l
Copy link
Contributor

@an3l an3l commented May 30, 2022

…er container

  • After setting up the virtual environment for the project
    (note missing instruction how to work locally with venv in Readme.md) and
    after running the makemigrations --dry-run or starting the webserver,
    application is trying to connect with hardcoded hostname db used from Dockerfile
  • This patch allows to add multiple hostname from environment variable separed
    by delimiter (I liked ,) and since the create type in settings.py is list type
    it is free to use first value from a list.
  • Proof
  • No patch
$ export DJANGO_ALLOWED_HOSTS=localhost,127.0.0.1
$ ./manage.py makemigrations --dry-run
/home/anel/mariadb/feedback-plugin-backend/env/lib/python3.8/site-packages/django/core/management/commands/makemigrations.py:105:
  RuntimeWarning: Got an error checking a consistent migration history performed for database connection 'default': (2005, "Unknown MySQL server host 'db' (-3)")
  warnings.warn(
Migrations for 'feedback_plugin':
  feedback_plugin/migrations/0002_alter_rawdata_upload_time.py
    - Alter field upload_time on rawdata
  • With patch
$ ./manage.py makemigrations --dry-run
Migrations for 'feedback_plugin':
  feedback_plugin/migrations/0002_alter_rawdata_upload_time.py
    - Alter field upload_time on rawdata
  • Also server gets started correctly
$ ./manage.py runserver
Performing system checks...

System check identified no issues (0 silenced).

You have 19 unapplied migration(s). Your project may not work properly until you apply the migrations for app(s): admin, auth, contenttypes, feedback_plugin, sessions.
Run 'python manage.py migrate' to apply them.
May 30, 2022 - 14:51:50
Django version 3.2.13, using settings 'feedback_plugin.settings'
Starting development server at http://127.0.0.1:8000/
Quit the server with CONTROL-C.

@an3l an3l changed the title Change default connection to database to use host different than dock… MDBF-423: Change default connection to database to use host different than dock… May 30, 2022
@grooverdan
Copy link
Member

DJANGO_ALLOWED_HOSTS and the database host are actually different concepts. Why not os.environ['DJANGO_DB_HOST']

Copy link
Member

@cvicentiu cvicentiu left a comment

Choose a reason for hiding this comment

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

@an3l I like the first change where one allows multiple hosts in DJANGO's allow hosts.
I am ok with having a change-able db host via an environment variable. You can do as Daniel suggests, but please also update

docker/.env file if you do so.

@an3l
Copy link
Contributor Author

an3l commented May 31, 2022

Ok based on documentation allowed_host is used for other purposes (may be empty when DEBUG=true).
Django documentation for setting up MySQL has an example of usage of configuration file in options over NAME, HOST, etc , however I don't want to write the tweaks to populate configuration file with variable parameters like environment databases.
Have updated the patch @cvicentiu

@an3l
Copy link
Contributor Author

an3l commented May 31, 2022

Based on #8 it seems that one need to have docker_db_1 as an default host based on the docker_default network and if needed user need to change manually to localhost (or empty string), atm I don't know other solution to have defaults both for container and host as localhost.
Instead when using locally this will be interpreted as an localhost, but when using in a container empty string will use socket in web_1 container and the host will not be visible, ever.

…er container

- After setting up the virtual environment for the project
  (note missing instruction how to work locally with venv in `Readme.md`) and
  after running the `makemigrations --dry-run` or starting the webserver,
  application is trying to connect with hardcoded hostname `db` used from Dockerfile
- This patch allows to add multiple hostname from environment variable `DJANGO_ALLOWED_HOSTS` separated
  by delimiter (I liked `,`) and since the create type in settings.py is `list` type
  it is free to use first value from a list if/when needed.
- Add `get()` method for `os.environ` which is returning `None` instead of `KeyError` and validate parameters that we need.
  Using this approach only `$ export DJANGO_DB_HOST DJANGO_USER_NAME DJANGO_USER_NAME_PASSWOR` is needed.
- Update `docker/.env` with new added environment variable `DJANGO_DB_HOST`
- Proof
* No patch
```
$ export DJANGO_ALLOWED_HOSTS=localhost,127.0.0.1
$ ./manage.py makemigrations --dry-run
/home/anel/mariadb/feedback-plugin-backend/env/lib/python3.8/site-packages/django/core/management/commands/makemigrations.py:105:
  RuntimeWarning: Got an error checking a consistent migration history performed for database connection 'default': (2005, "Unknown MySQL server host 'db' (-3)")
  warnings.warn(
Migrations for 'feedback_plugin':
  feedback_plugin/migrations/0002_alter_rawdata_upload_time.py
    - Alter field upload_time on rawdata
```
* With patch
```
$ ./manage.py makemigrations --dry-run
Migrations for 'feedback_plugin':
  feedback_plugin/migrations/0002_alter_rawdata_upload_time.py
    - Alter field upload_time on rawdata
```
- Also server gets started correctly
```
$ ./manage.py runserver
Performing system checks...

System check identified no issues (0 silenced).

You have 19 unapplied migration(s). Your project may not work properly until you apply the migrations for app(s): admin, auth, contenttypes, feedback_plugin, sessions.
Run 'python manage.py migrate' to apply them.
May 30, 2022 - 14:51:50
Django version 3.2.13, using settings 'feedback_plugin.settings'
Starting development server at http://127.0.0.1:8000/
Quit the server with CONTROL-C.
```
@an3l an3l force-pushed the anel-host_name branch from d6c14b2 to 0e59c81 Compare June 1, 2022 09:35
@@ -29,6 +29,7 @@ DJANGO_DEBUG=True
#
# There will also be a test_{DJANGO_DB_NAME} database used for running tests.
DJANGO_DB_NAME='feedback_plugin'
DJANGO_DB_HOST='db'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on #8 it seems that one need to have docker_db_1 as an default host based on the docker_default network

# DB_HOST default means localhost, instead one should set the unix socket and/or other host
DB_HOST = os.environ.get('DJANGO_DB_HOST')
if DB_HOST == None:
DB_HOST = ''
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same is here ^^^
Based on #8 it seems that one need to have docker_db_1 as an default host based on the docker_default network
Instead when using locally this will be interpreted as an localhost, but when using in a container empty string will use socket in web_1 container and the host will not be visible, ever.
This will need to update.

@cvicentiu
Copy link
Member

Closing this as currently the deployment relies on docker-compose for full stack deployment. We will make these changes when we need to have a DB run on a different host.

We will keep using square brackets for environment variable fetching to highlight bugs. If an environment variable is missing, we want the code to throw an error, not silently continue.

@cvicentiu cvicentiu closed this Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants