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.

Sign up for GitHub

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

Jump to bottom

Customize columns, drop callback, create table is optional #70

Merged
merged 7 commits into from
Jul 3, 2020
Merged

Customize columns, drop callback, create table is optional #70

merged 7 commits into from
Jul 3, 2020

Conversation

nadege
Copy link
Contributor

@nadege nadege commented Jun 26, 2020

Cf. https://people-doc.atlassian.net/browse/PY-32

Ok I hope this will the last PR to allow septentrion to be used in peopledoc/django-north

It contains:

  • customization of migration table columns names (for compatibility with django_migration table)
  • drop the callback feature called on applied migration (septentrion writes in the migration itself)
  • add option to no create the migration table (in case creation is handled by a migration!)

Successful PR Checklist:

@codecov
Copy link

codecov bot commented Jun 26, 2020 •
edited
Loading

Codecov Report

Merging #70 into master will increase coverage by 1.71%.
The diff coverage is 73.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage   74.70%   76.41%   +1.71%     
==========================================
  Files          15       15              
  Lines         676      687      +11     
  Branches       92       92              
==========================================
+ Hits          505      525      +20     
+ Misses        158      147      -11     
- Partials       13       15       +2
Flag Coverage Δ
#integration 49.49% <34.61%> (+1.70%) ⬆️
#unit 63.75% <53.84%> (-0.30%) ⬇️
Impacted Files Coverage Δ
septentrion/cli.py 0.00% <0.00%> (ø)
septentrion/configuration.py 83.14% <ø> (ø)
septentrion/migration.py 79.34% <0.00%> (-1.30%) ⬇️
septentrion/core.py 77.21% <50.00%> (-0.99%) ⬇️
septentrion/db.py 95.77% <100.00%> (+23.89%) ⬆️
septentrion/lib.py 82.50% <100.00%> (ø)

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 ae8202f...15f1e8b. Read the comment docs.

@nadege
Copy link
Contributor Author

nadege commented Jun 26, 2020

Ready for review

k4nar
k4nar reviewed Jun 26, 2020
View reviewed changes
septentrion/db.py Outdated
version_column=settings.VERSION_COLUMN,
name_column=settings.NAME_COLUMN,
applied_column=settings.APPLIED_COLUMN,
).split()
Copy link
Contributor

Choose a reason for hiding this comment

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

Dumb question, why do we need to split & join the query? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to remove line breaks.

in the past we had : 76138bb#diff-dcf2e67bd860136fb966a5112a6ab275L17

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to remove line break, then ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You wrote the original code, so you tell me :)
Maybe it's not useful anymore, I didn't dig into that, as it's not the subject of that PR...

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, it's hard to remember things from 2 years ago.

My guess would be a similar project I was inspired from was doing the same. I'm trying to remember if we were working on postgres tools at the time...

Today, my bet would be: if nothing justifies it, let's get rid of it. If we needed, we'll see it soon enough and re-add it with a comment !

Copy link
Contributor

Choose a reason for hiding this comment

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

Clarifying! Let's get rid of it in another PR, some other time #71

ewjoachim
ewjoachim reviewed Jun 30, 2020
View reviewed changes
septentrion/cli.py Outdated Show resolved Hide resolved
ewjoachim
ewjoachim reviewed Jun 30, 2020
View reviewed changes
septentrion/cli.py Outdated Show resolved Hide resolved
ewjoachim
ewjoachim reviewed Jun 30, 2020
View reviewed changes
septentrion/cli.py Outdated Show resolved Hide resolved
ewjoachim
ewjoachim reviewed Jun 30, 2020
View reviewed changes
septentrion/cli.py Outdated Show resolved Hide resolved
ewjoachim
ewjoachim reviewed Jun 30, 2020
View reviewed changes
septentrion/db.py Outdated
Comment on lines 159 to 162
with Query(settings=settings, query=query_migration_table_exists) as cur:
result = next(cur)
if result == [False]:
return False
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have "CREATE IF NOT EXISTS", I'm not sure what this brings to the table...? Couldn't we just always try to create the table (if not --no-create-table) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand your comment, as this function is not about table creation.

ewjoachim
ewjoachim reviewed Jun 30, 2020
View reviewed changes
septentrion/cli.py Outdated Show resolved Hide resolved
@nadege
Copy link
Contributor Author

nadege commented Jun 30, 2020

Integrated wording changes and create table option fix.

Also, can we name it applied_at rather than applied which sounds like a boolean?

I still have to do that.

ewjoachim
ewjoachim approved these changes Jul 1, 2020
View reviewed changes
Copy link
Contributor

@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.

Yay, 👏 ! Really really nice 🎉

(sorry it took some time to finish the review)

Note: I still need to add tests for the commits I added at the end. I'll be doing this asap !

@ewjoachim ewjoachim force-pushed the customize-columns branch 2 times, most recently from 9a0ef8a to 35c143e Compare July 1, 2020 08:57
Nadège Michel and others added 6 commits July 1, 2020 11:47
Customize migrations table columns names
f529d87
Migration table creation is optional
84eb76b
Remove callback feature
5faa26e
@nadege
Improve options wording and fix "[no] create table" option.
f1ece33 
Co-authored-by: Joachim Jablon <[email protected]>
Refactor is_schema_initialized
3e297c9 
Instead of checking that the migration table exists, assume it does and
catch the exception
Remove potential SQL injection
c2eb024 
We'll make this code cleaner in a subsequent PR
ewjoachim
ewjoachim approved these changes Jul 3, 2020
View reviewed changes
Copy link
Contributor

@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.

Congratulation on a non-trivial PR ! \o/

@ewjoachim ewjoachim merged commit 71a4691 into master Jul 3, 2020
@ewjoachim ewjoachim deleted the customize-columns branch July 3, 2020 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@k4nar k4nar k4nar left review comments

@ewjoachim ewjoachim ewjoachim approved these changes

@mgu mgu Awaiting requested review from mgu

@samuelhamard samuelhamard Awaiting requested review from samuelhamard

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nadege @ewjoachim @k4nar