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 contrib app #283

Merged
merged 18 commits into from
Aug 19, 2020
Merged

Django contrib app #283

merged 18 commits into from
Aug 19, 2020

Conversation

agateblue
Copy link
Contributor

Closes #280

Successful PR Checklist:

  • Tests
  • Documentation
  • Had a good time contributing?
  • (Maintainers: add PR labels)

I'll add documentation if the PR is considered okay, but basically:

  1. Add "procrastinate.contrib.django", to INSTALLED_APPS
  2. Run python manage.py migrate

Let me know if you have any question!

cursor.execute("SELECT * FROM procrastinate_jobs")


def test_no_missing_migration():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a safeguard test to ensure we never add a .sql migration without its .py counterpart

@codecov
Copy link

codecov bot commented Jul 18, 2020

Codecov Report

Merging #283 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #283   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        39   +13     
  Lines         1425      1488   +63     
  Branches       162       162           
=========================================
+ Hits          1425      1488   +63     
Impacted Files Coverage Δ
procrastinate/contrib/django/__init__.py 100.00% <100.00%> (ø)
procrastinate/contrib/django/apps.py 100.00% <100.00%> (ø)
...astinate/contrib/django/migrations/0001_initial.py 100.00% <100.00%> (ø)
...b/django/migrations/0002_drop_started_at_column.py 100.00% <100.00%> (ø)
...b/django/migrations/0003_drop_started_at_column.py 100.00% <100.00%> (ø)
...igrations/0004_drop_procrastinate_version_table.py 100.00% <100.00%> (ø)
...ngo/migrations/0005_fix_procrastinate_fetch_job.py 100.00% <100.00%> (ø)
...igrations/0006_fix_trigger_status_events_insert.py 100.00% <100.00%> (ø)
...django/migrations/0007_add_queueing_lock_column.py 100.00% <100.00%> (ø)
.../migrations/0008_close_fetch_job_race_condition.py 100.00% <100.00%> (ø)
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0bb83f...45a4ddd. Read the comment docs.

Copy link
Member

@ewjoachim ewjoachim left a comment

Choose a reason for hiding this comment

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

That's a very promising start ! A few comments below, and, as you noted, the documentation (we can help with that).

Do you think it's worth generating Django migration files, or do you think documenting the process will be enough ?

procrastinate/contrib/django/utils.py Outdated Show resolved Hide resolved
tests/acceptance/test_django.py Outdated Show resolved Hide resolved
tests/acceptance/test_django.py Outdated Show resolved Hide resolved
tests/unit/test_django_utils.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@ewjoachim
Copy link
Member

Splendid! I think your proposal goes in the right direction.

I'll let you be the judge for the following: I don't want to delay the whole thing just because of my silly suggestions, so if you think it makes sense, we could split that into 2 PRs: one without automated generation (or with automated generation but not triggered by setup.py) and one with fully automated generation in setup.py, which seems to be not as simple. You decide, really, I'm fine with both.

@ewjoachim
Copy link
Member

Hey :) Just want to make sure everything is all right regarding this contribution. You're welcome to take all the time you need to iterate on the different parts, but if you run into a blocker or have a part you don't want to invest your time into, it's very ok not to. Just make sure to let me know if/how I can help.

See you around!

@agateblue
Copy link
Contributor Author

Hey @ewjoachim !

I've lacked the spoons lately, but I do want to get back at it and complete it, probably without automation :)

I hope you're okay, take care

@ewjoachim
Copy link
Member

ewjoachim commented Jul 31, 2020

Hey :)

Here's a handful of spoons 🥄*💯
(for anyone wondering what those two strange individuals are talking about, have a look at this)

Take your time, it's ok. Wishing you all the best and 10% more on top of that.

See you around, take care :)

@agateblue
Copy link
Contributor Author

So I reverted the dynamic migration generation and integrated your suggestions:

  • [django] extra
  • Moving functions / tests in different modules for consistency

However, the get_sql call to read_text fails with:

FileNotFoundError: 'baseline-0.5.0.sql' resource not found in 'procrastinate.sql.migrations'

I'm not familiar at all with importlib_resources so maybe you'll have an idea @ewjoachim ?

@ewjoachim
Copy link
Member

Hey there :) Sorry for the response time, I'm going to have a look. Thanks for taking the time!

@ewjoachim
Copy link
Member

ewjoachim commented Aug 18, 2020

First approximation: I think it's because read_text only works with python modules, so the migrations folder needs an empty __init__.py file. When I add it, it seems I have a test error further down the line, so this could be it!

Copy link
Member

@ewjoachim ewjoachim left a comment

Choose a reason for hiding this comment

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

We're almost there ! Thank a lot for your work so far :)

tests/unit/test_schema.py Outdated Show resolved Hide resolved
procrastinate/contrib/django/migrations/0001_initial.py Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
tests/acceptance/django_settings.py Outdated Show resolved Hide resolved
tests/migration/test_migration.py Outdated Show resolved Hide resolved
tests/migration/test_migration.py Outdated Show resolved Hide resolved
Copy link
Member

@ewjoachim ewjoachim left a comment

Choose a reason for hiding this comment

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

woops forgotten comment.

procrastinate/contrib/django/apps.py Outdated Show resolved Hide resolved
@agateblue
Copy link
Contributor Author

I've integrated your suggestions @ewjoachim, and the test suite seems to pass \o/

Copy link
Member

@ewjoachim ewjoachim left a comment

Choose a reason for hiding this comment

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

Aaaaand it's a wrap 🎬

Congratulations :)

If you want to continue contributing, let me know, I'm sure we can find interesting avenues :) Feel free to have a look at the issues anyway !

@ewjoachim ewjoachim merged commit b703bb6 into procrastinate-org:master Aug 19, 2020
@agateblue agateblue deleted the 280-django branch August 19, 2020 08:25
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.

Best way to integrate with django migration system
2 participants