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

Django initialized, connected to postgres. #72

Open
wants to merge 4 commits into
base: django
Choose a base branch
from
Open

Conversation

latachz
Copy link
Member

@latachz latachz commented Feb 28, 2020

I've created Django app with connection to postgresql and substitution model.

Copy link
Member

@pniedzwiedzinski pniedzwiedzinski left a comment

Choose a reason for hiding this comment

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

Dockerfile.dev is not present here but we need it 😿 It should be okay if you just copy it from the develop/master

@@ -0,0 +1,23 @@
FROM python:3.8-alpine
Copy link
Member

Choose a reason for hiding this comment

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

Strange file name

backend/requirements.txt Outdated Show resolved Hide resolved
@@ -17,6 +17,6 @@ services:
context: backend
dockerfile: Dockerfile.dev
ports:
- 5000:5000
- 8000:8000
Copy link
Member

Choose a reason for hiding this comment

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

The backend is actually ported through nginx, so we need to update also the nginx configuration.

'NAME': 'substitutions',
'USER': 'postgres',
'PASSWORD': 'postgres',
'HOST': 'localhost',
Copy link
Member

Choose a reason for hiding this comment

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

Let's use docker built-in DNS so we don't need to bind Postgres to the host port. This will prevent accessing the Postgres instance from the local network - which we don't need and don't want. Also add postgres to docker-compose. If you don't know how docker inner routing works I have a great book for that 😉 if you're interested...

Suggested change
'HOST': 'localhost',
'HOST': 'postgres',

backend/main/models.py Outdated Show resolved Hide resolved
Copy link
Member

@pniedzwiedzinski pniedzwiedzinski left a comment

Choose a reason for hiding this comment

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

Also added a couple of comments about the model

('class_tag', models.CharField(max_length=30)),
('subject_name', models.CharField(max_length=30)),
('classroom', models.CharField(max_length=30)),
('substitute_teacher', models.CharField(max_length=30)),
Copy link
Member

Choose a reason for hiding this comment

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

Date not present in initial migration, just add it manually


class SubstitutionModel(models.Model):
lesson_number = models.IntegerField()
absent_teacher = models.CharField(max_length=30)
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be 60:

  • 30 - name
  • 30 - last name

class SubstitutionModel(models.Model):
lesson_number = models.IntegerField()
absent_teacher = models.CharField(max_length=30)
class_tag = models.CharField(max_length=30)
Copy link
Member

Choose a reason for hiding this comment

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

1GLa - that's 4, but to be safe let's add 8

absent_teacher = models.CharField(max_length=30)
class_tag = models.CharField(max_length=30)
subject_name = models.CharField(max_length=30)
classroom = models.CharField(max_length=30)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that classroom id will be ever longer than 8

class_tag = models.CharField(max_length=30)
subject_name = models.CharField(max_length=30)
classroom = models.CharField(max_length=30)
substitute_teacher = models.CharField(max_length=30)
Copy link
Member

Choose a reason for hiding this comment

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

as above

volumes:
- ./backend:/app
db:
Copy link
Member

Choose a reason for hiding this comment

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

Mismatch of db key in docker-compose and postgres host name from the django setting


LANGUAGE_CODE = 'en-us'

TIME_ZONE = 'UTC'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TIME_ZONE = 'UTC'
TIME_ZONE = 'Europe/Warsaw'

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