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

Disregard migrations older than the schema #81

Merged
merged 1 commit into from
Jul 24, 2020
Merged

Conversation

ewjoachim
Copy link
Contributor

Cf. #32

With this PR, migrations older than the schema are not considered. If no schema is explicitely configured (even if one is used, because the latest schema is automatically used), then all migrations are considered.

Is it the right thing to do, or should I disregard all migrations older than the schema event when schema is not explicitely provided ?

Missing tests (& doc but the Septentrion doc is... Not ready)

Successful PR Checklist:

  • Tests
  • Documentation (optionally: run spell checking)
  • Had a good time contributing? (feel free to give some feedback)

@ewjoachim ewjoachim requested a review from nadege July 23, 2020 10:07
@nadege
Copy link
Contributor

nadege commented Jul 23, 2020

I think we should disregard migrations older than the schemas in all cases. The schemas should represent what the older migrations would have output.

@codecov
Copy link

codecov bot commented Jul 23, 2020

Codecov Report

Merging #81 into master will decrease coverage by 0.54%.
The diff coverage is 56.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
- Coverage   79.17%   78.62%   -0.55%     
==========================================
  Files          15       15              
  Lines         629      641      +12     
  Branches       79       81       +2     
==========================================
+ Hits          498      504       +6     
- Misses        122      127       +5     
- Partials        9       10       +1     
Flag Coverage Δ
#integration 57.87% <31.25%> (-0.79%) ⬇️
#unit 60.84% <43.75%> (-0.21%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
septentrion/core.py 72.09% <22.22%> (-5.13%) ⬇️
septentrion/migration.py 80.21% <100.00%> (ø)
septentrion/utils.py 100.00% <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 8e8f5c8...60a6fe6. Read the comment docs.

@ewjoachim ewjoachim requested a review from mgu July 23, 2020 10:34
@mgu
Copy link
Contributor

mgu commented Jul 23, 2020

I fully agree with @nadege :)

@ewjoachim
Copy link
Contributor Author

Ah, that's a good thing that I had gone ahead and implemented @nadege 's suggestion, then :)

It's ready to review !

@ewjoachim ewjoachim marked this pull request as ready for review July 23, 2020 14:21
@@ -128,7 +128,9 @@ def get_fixtures_version(
return version


def build_migration_plan(settings: configuration.Settings) -> Iterable[Dict[str, Any]]:
def build_migration_plan(
settings: configuration.Settings, schema_version: Optional[versions.Version] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

why is the schema_version parameter optional ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I thought it was the case, but it later turned out not to be. I'll remove the default at None too, event if it means I have to fix a few tests.

Copy link
Contributor

@mgu mgu left a comment

Choose a reason for hiding this comment

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

🎉
LGTM

@ewjoachim ewjoachim merged commit 100f23a into master Jul 24, 2020
@ewjoachim ewjoachim deleted the old-migrations branch July 24, 2020 07:26
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.

3 participants