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

Merge current migration before release to reduce number of migrations that must occur #48

Closed
heartsucker opened this issue Oct 18, 2018 · 7 comments
Assignees

Comments

@heartsucker
Copy link
Contributor

Additionally, if we implement #47, then we will need to rewrite these anyway to include the new key names.

@redshiftzero
Copy link
Contributor

redshiftzero commented Oct 18, 2018

yep, we should definitely squash migrations prior to beta

@redshiftzero redshiftzero added this to the 0.1beta milestone Oct 24, 2018
@redshiftzero redshiftzero added the help wanted Extra attention is needed label Nov 5, 2018
@heartsucker
Copy link
Contributor Author

Note that if #112 is merged, we must implement this before the beta is rolled out.

@heartsucker
Copy link
Contributor Author

I want to reopen this for a couple reasons.

First, there was an annoying error in the first migration, namely that we defined a constraint on a column in the migration as ck_replies_is_downloaded but this should have been defined as either op.f(ck_replies_is_downloaded or simply is_downloaded. Failing to do so created a constraint named ck_replies_ck_replies_is_downloaded. This is not inline with our naming convention and should be migrated to line up otherwise future migrations will need to remember this.

Additionally in order to support #203, #207, and #170, we need to write some annoying migrations that require dropping tables and either bulk or iteratively inserting columns. These lead to a significant increase in complexity for implementing #55.

The reason discussed with @redshiftzero for adding migrations as new features come in (namely for adding the fingerprint column as part of #190) was to support devs and auditors who are using this via the deb package (#190 (comment)). I really think it's worth while to tell devs / auditors to just blow away their local db in order to make this change to prevent breakage and code complexity this early in the lifetime of this project.

@heartsucker heartsucker reopened this Dec 5, 2018
@heartsucker
Copy link
Contributor Author

For reference, see the change on this branch: https://github.com/freedomofpress/securedrop-client/tree/alembic-tests

@redshiftzero
Copy link
Contributor

Ok we can do that since I'm with you that it is pretty cumbersome to write lots of table-dropping SQL migrations, but we should triple check that we are happy with the database structure for the beta release as it will be set in stone as it were. Auditors will be 100% done in a week-ish which means that the only people that need to blow away their client directory are the small number of developers using this on Qubes (let's just ask people to do this once), which I think is acceptable.

@heartsucker heartsucker self-assigned this Dec 6, 2018
@heartsucker heartsucker removed the help wanted Extra attention is needed label Dec 19, 2018
@heartsucker
Copy link
Contributor Author

Note that this likely depends on #217.

@redshiftzero
Copy link
Contributor

Closing this since we are adding database changes to the now single migration script for beta here: https://github.com/freedomofpress/securedrop-client/blob/master/alembic/versions/2f363b3d680e_init.py

We also have a note in the README generically describing the process of merging migrations before releases here: https://github.com/freedomofpress/securedrop-client#merging-migrations

legoktm pushed a commit that referenced this issue Dec 11, 2023
ci: ensure that requirements files are in sync
legoktm pushed a commit that referenced this issue Dec 11, 2023
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

No branches or pull requests

2 participants