wemake-services / wemake-django-template

Bleeding edge django template focused on code quality and security.
https://wemake-django-template.rtfd.io
MIT License
2k stars 215 forks source link

Enable `django_checks` check from `django-test-migrations` #1384

Open sobolevn opened 3 years ago

sobolevn commented 3 years ago

https://github.com/wemake-services/django-test-migrations/blob/master/django_test_migrations/contrib/django_checks.py#L47

@skarzi are you interested in helping me out?

skarzi commented 3 years ago

sure! I can prepare the initial PR for this issue on the weekend

sobolevn commented 3 years ago

Awesome, thanks!

skarzi commented 3 years ago

I have tested django-test-migrations from master branch with wemake-django-template on my fork issue-1384 branch.

Things to do/discuss:

  1. DatabaseConfiguration checks should be mainly run before deployment (connected to production DB), so we can check that the production database is properly configured, but this will require defining django-test-migration as the main dependency. Personally, I dislike putting dependencies with test in their names to main ones, what do you think about it? Do you have any other ideas?
  2. We need to release a new version of django-test-migrations, but it's necessary to fix MySQL support - check https://github.com/wemake-services/django-test-migrations/issues/122 for more details
sobolevn commented 3 years ago

DatabaseConfiguration checks should be mainly run before deployment (connected to production DB), so we can check that the production database is properly configured, but this will require defining django-test-migration as the main dependency. Personally, I dislike putting dependencies with test in their names to main ones, what do you think about it? Do you have any other ideas?

Currently it lives in development only 🤔 https://github.com/wemake-services/wemake-django-template/blob/master/%7B%7Bcookiecutter.project_name%7D%7D/server/settings/environments/development.py#L33

I guess that adding all django-test-migrations to prod dependencies just because of this one feature is not a good idea for our template.

It might be useful for some people, but in this case it is up to them to decide. I will add a short doc comment about it.

skarzi commented 3 years ago

I guess that adding all django-test-migrations to prod dependencies just because of this one feature is not a good idea for our template.

I totally agree. I just started thinking about moving our checks to django-extra-checks and leave django-test-migrations with only one responsibility - migrations testing, What's your opinion on that?

sobolevn commented 3 years ago

Hm, this way we would have django-extra-checks as a main dependency. But, it really has a lot of dev-only checks. Like:

So, this is not really a change.

Maybe we can create a new lib? Something like django-pre-deploy-checks?

This way we can clearly indicate that these checks are only meant to be executed before the actual deploy. Ideas:

How do you like it? If you like the idea, I will create some initial boilerplate and start things up!

skarzi commented 3 years ago

Maybe we can create a new lib? Something like django-pre-deploy-checks?

That's a great idea, let's give it a go!

sobolevn commented 3 years ago

Done! https://github.com/wemake-services/django-pre-deploy-checks

Added you as a maintainer 👍

skarzi commented 3 years ago

super :+1: I will try to move all required code from django-test-migrations to the new package on the weekend