Skip to content

Database migration with alembic#112

Merged
frgfm merged 9 commits into
masterfrom
dbmigration-alembic
Mar 24, 2021
Merged

Database migration with alembic#112
frgfm merged 9 commits into
masterfrom
dbmigration-alembic

Conversation

@jean-pasquier

@jean-pasquier jean-pasquier commented Feb 22, 2021

Copy link
Copy Markdown
Contributor

Database migration implementation using Alembic tool: alter database schema (create table, add column, etc.) using SQLAlchemy syntax, more info in src/alembic/README.md

  • src/alembic.ini is configuration (almost default one, generated by alembic)
  • src/alembic/
    • versions/ contains migration scripts (so far empty)
    • env.py alembic migration skeletton (default adapted to retrieve database URL from the app config)
    • README.md alembic generated an empty readme, first draft here
    • script.py.mako script generation template (default one)

Open questions:

  • alembic added to requirements.txt, afterward maybe it should be in requirements-dev.txt since it concerns CI/CD phase and not running docker image ?
  • add a Github workflow to run the migration in production, only once it is merged to master ?
  • how to test the scripts before merging to master ? There are libraries such as pytest-alembic
  • encode (maintainers of async database package) use another but similar method to set database url config.set_main_option in there doc

resolves #107

@jean-pasquier jean-pasquier added topic: docs Improvements or additions to documentation type: improvement New feature or request help wanted Extra attention is needed high priority labels Feb 22, 2021
@jean-pasquier jean-pasquier self-assigned this Feb 22, 2021
@jean-pasquier
jean-pasquier marked this pull request as draft February 23, 2021 10:28
frgfm
frgfm reviewed Feb 26, 2021
View reviewed changes

@frgfm frgfm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this PR! I added a few comments, but I have no experience with alembic

Comment thread requirements.txt Outdated
Comment thread Dockerfile
Comment thread src/alembic.ini Outdated
Comment thread src/alembic/README.md Outdated
Comment thread .github/workflows/main.yml Outdated
fe51
fe51 previously approved these changes Mar 10, 2021
View reviewed changes
@jean-pasquier
jean-pasquier marked this pull request as ready for review March 17, 2021 21:54
frgfm
frgfm previously approved these changes Mar 17, 2021
View reviewed changes

@frgfm frgfm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the edits! I'm still having doubts for the Dockerfile modifications but it's nitpicking

frgfm
frgfm reviewed Mar 17, 2021
View reviewed changes

@frgfm frgfm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just the copyright & license notice missing 👌

Comment thread src/alembic/env.py
fe51
fe51 previously approved these changes Mar 18, 2021
View reviewed changes
@fe51 fe51 mentioned this pull request Mar 18, 2021
Closed
@frgfm
frgfm dismissed stale reviews from fe51 and themself via 5271767 March 23, 2021 21:52
frgfm
frgfm approved these changes Mar 23, 2021
View reviewed changes

@frgfm frgfm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All good now!

@frgfm frgfm added this to the 0.1.2 milestone Mar 23, 2021
@frgfm frgfm mentioned this pull request Mar 23, 2021
Closed
@codecov

codecov Bot commented Mar 23, 2021

Copy link
Copy Markdown

Codecov Report

Merging #112 (5271767) into master (a139a8c) will increase coverage by 0.16%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
+ Coverage   88.31%   88.47%   +0.16%     
==========================================
  Files          27       27              
  Lines         787      807      +20     
==========================================
+ Hits          695      714      +19     
- Misses         92       93       +1
Flag Coverage Δ
unittests 88.47% <ø> (+0.16%) ⬆️

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

Impacted Files Coverage Δ
src/app/api/crud/base.py 98.14% <0.00%> (-1.86%) ⬇️
src/app/db/tables.py 100.00% <0.00%> (ø)
src/app/api/routes/sites.py 100.00% <0.00%> (ø)
src/app/api/routes/events.py 100.00% <0.00%> (ø)
src/app/api/routes/devices.py 100.00% <0.00%> (ø)
src/app/api/routes/accesses.py 100.00% <0.00%> (ø)
src/app/api/schemas.py 99.15% <0.00%> (+0.02%) ⬆️

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 a139a8c...5271767. Read the comment docs.

@frgfm

frgfm commented Mar 24, 2021

Copy link
Copy Markdown
Member

@jeanpasquier75 I think we are ready to merge. Since we don't have a unittest or any CI integration yet, can you confirm that you tried to modify one of the table and used your alembic instructions to perform the migration, which was successful ? :)

@jean-pasquier

jean-pasquier commented Mar 24, 2021

Copy link
Copy Markdown
Contributor Author

@jeanpasquier75 I think we are ready to merge. Since we don't have a unittest or any CI integration yet, can you confirm that you tried to modify one of the table and used your alembic instructions to perform the migration, which was successful ? :)

Yes sir I confirm that it works fine in my postgres image 😉

@frgfm
frgfm merged commit 1e13ed8 into master Mar 24, 2021
@frgfm
frgfm deleted the dbmigration-alembic branch March 24, 2021 17:44
@frgfm frgfm added type: feat topic: migration and removed type: improvement New feature or request labels Jun 16, 2022
@frgfm frgfm mentioned this pull request Oct 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@frgfm frgfm frgfm approved these changes
@fe51 fe51 fe51 approved these changes
@florianriche florianriche Awaiting requested review from florianriche
@chloeskt chloeskt Awaiting requested review from chloeskt
@martin1tab martin1tab Awaiting requested review from martin1tab

Assignees

@jean-pasquier jean-pasquier

Labels

help wanted Extra attention is needed high priority topic: docs Improvements or additions to documentation topic: migration type: feat

Projects

None yet

Milestone

0.1.2

Development

Successfully merging this pull request may close these issues.

[database] Plan a migration process for the DB

3 participants

@jean-pasquier @frgfm @fe51