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

alembic update #96

Merged
merged 2 commits into from
Feb 10, 2021
Merged

alembic update #96

merged 2 commits into from
Feb 10, 2021

Conversation

ehenneken
Copy link
Member

  • Cleanup of Alembic configuration
  • Addition of two database columns

@coveralls
Copy link

coveralls commented Feb 9, 2021

Coverage Status

Coverage increased (+0.01%) to 99.291% when pulling 55edae0 on ehenneken:db_update into 8216bb5 on adsabs:master.

Copy link
Contributor

@marblestation marblestation left a comment

Choose a reason for hiding this comment

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

It looks good! I am only uncertain about how alembic will behave with the new scripts:

  • Revision numbers: I added a small suggestion to make it more in line with what we had before (I believe alembic relies on the revision numbers, so better to conserve what we already had)
  • Drop table: Maybe you have already tested that alembic will not drop the table when it will be run in dev/prod, but out of caution (and my own ignorance), I added a comment to double check

These are just suggestions to verify before deployment, so as long as you feel confident with it, we can go ahead!

Comment on lines +35 to +39
def downgrade():
# ### commands auto generated by Alembic - please adjust! ###
op.drop_index(op.f('ix_graphics_bibcode'), table_name='graphics')
op.drop_table('graphics')
# ### end Alembic commands ###
Copy link
Contributor

Choose a reason for hiding this comment

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

This alembic scripts didn't exist before and the only one that existed (revision number 437251c0dc94) has been removed in this PR:

revision = '437251c0dc94'

I do not know if alembic will misbehave due to missing revision number + new version scripts and delete the full graphics table. Probably not, but I just wanted to mention that I am not certain. Maybe you have already verified this?



# revision identifiers, used by Alembic.
revision = 'c36480cd22db'
Copy link
Contributor

Choose a reason for hiding this comment

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

We had a similar file before that has been deleted instead of moved:

revision = '437251c0dc94'

Could we at least rename the new one to match the previous one and change the revision (437251c0dc94 instead of c36480cd22db) to be the same as it was? This will also require to update:

down_revision = 'c36480cd22db'

in the file alembic/versions/b9308d306405_create_db_tables.py. This is probably not super important, I am just unsure of the side effects.

@ehenneken ehenneken merged commit fd8975d into adsabs:master Feb 10, 2021
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.

None yet

3 participants