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

Handle foreign keys and indices properly in SQLite and add e2e tests #94

Merged
merged 1 commit into from
Jun 23, 2020
Merged

Handle foreign keys and indices properly in SQLite and add e2e tests #94

merged 1 commit into from
Jun 23, 2020

Conversation

aeneasr
Copy link
Member

@aeneasr aeneasr commented Jun 19, 2020 •
edited
Loading

This patch resolves issues with change_column and drop_column which previously caused duplicate or missing indices and foreign keys. To prevent future regressions and help debugging,
a new e2e-test pipeline has been added to the project and the CI. Documentation for running the tests has been added to the README.

End-to-end tests currently support MySQL, PostgreSQL, and SQLite and do not yet cover other SQL dialects.

During review, please help ensure that the SQL fixtures are not missing something :)

Closes #92

@aeneasr aeneasr requested a review from a team as a code owner June 19, 2020 12:28
@aeneasr aeneasr marked this pull request as draft June 19, 2020 12:28
@aeneasr
Copy link
Member Author

aeneasr commented Jun 19, 2020

CRDB tests fail because pgx driver is missing, but the important failing test case is this one: https://github.com/gobuffalo/fizz/pull/94/checks?check_run_id=788056231

@aeneasr
Copy link
Member Author

aeneasr commented Jun 19, 2020

What I don't really get, it appears that fizz.AString correctly includes the foreign key when using drop_column (see here) but the integration test (actually running the migration) does not include the foreign key. Would be great if I could get some help in understanding why!

@aeneasr aeneasr force-pushed the fix-92 branch 4 times, most recently from a109105 to 4f21a5e Compare June 20, 2020 13:01
@aeneasr aeneasr changed the title Add e2e tests and failing test case for #92 Handle foreign keys and indices properly in SQLite and add e2e tests Jun 20, 2020
@aeneasr
Copy link
Member Author

aeneasr commented Jun 20, 2020

@stanislas-m so, I've added a couple of e2e tests now, and fixed #92 and tests are all passing locally. This is ready for review :)

@aeneasr aeneasr marked this pull request as ready for review June 20, 2020 13:03
@aeneasr aeneasr force-pushed the fix-92 branch 2 times, most recently from cc0e2a1 to 3615967 Compare June 20, 2020 13:10
@aeneasr
Copy link
Member Author

aeneasr commented Jun 20, 2020

The failing CRDB test is unrelated to this changeset, it fails because soda is missing pgx. This is because fizz uses pgx on master already for e.g. CockroachDB (because of the meta code). Since pgx isn't merged to master in pop, soda fails with this code base. Once pgx is merged on master in pop, this test failure should go away.

@aeneasr aeneasr force-pushed the fix-92 branch 2 times, most recently from 6d07bc7 to d97f28c Compare June 20, 2020 13:24
@aeneasr aeneasr mentioned this pull request Jun 20, 2020
Closed
@stanislas-m stanislas-m added the s: blocked is blocked by the other issues/PRs label Jun 20, 2020
@aeneasr aeneasr mentioned this pull request Jun 21, 2020
Merged
@stanislas-m stanislas-m removed the s: blocked is blocked by the other issues/PRs label Jun 21, 2020
@aeneasr
Copy link
Member Author

aeneasr commented Jun 21, 2020

sqlite3 binary is missing on the windows system, any idea how to install it?

@aeneasr aeneasr force-pushed the fix-92 branch 3 times, most recently from 3e669bc to 4661e59 Compare June 21, 2020 20:16
@stanislas-m
Copy link
Member

Chocolatey should be available on Windows, so: choco install sqlite

@aeneasr
Copy link
Member Author

aeneasr commented Jun 21, 2020

Yup, that seems to have worked! :)

@aeneasr
Copy link
Member Author

aeneasr commented Jun 21, 2020

I think you need to update the repository required checks because I changed the job to a dedicated job for windows on SQLite (chocolatey is not available on the other systems) - it's still waiting for the old SQLite test

@aeneasr
Handle foreign keys and indices properly in SQLite and add e2e tests
1d7ad9d 
This patch resolves issues with `change_column` and `drop_column` which previously caused duplicate or missing indices and foreign keys. To prevent future regressions and help debugging,
a new e2e-test pipeline has been added to the project and the CI. Documentation for running the tests has been added to the README.

End-to-end tests currently support MySQL, PostgreSQL, and SQLite and do not yet cover other SQL dialects.

During review, please help ensure that the SQL fixtures are not missing something :)

Closes gobuffalo#92
@aeneasr
Copy link
Member Author

aeneasr commented Jun 21, 2020

Ok, figured out how to circumvent that, everything green :)

@aeneasr aeneasr mentioned this pull request Jun 22, 2020
Closed
@stanislas-m
Copy link
Member

LGTM, thanks!

@stanislas-m stanislas-m merged commit c8d2f47 into gobuffalo:master Jun 23, 2020
@sio4 sio4 mentioned this pull request Jul 9, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stanislas-m stanislas-m stanislas-m approved these changes

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

Successfully merging this pull request may close these issues.

Migrations are not idempotent and cause critical divergence between applies
2 participants
@aeneasr @stanislas-m