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

Implement drop table migrations #4

Merged
merged 27 commits into from
May 17, 2017
Merged

Implement drop table migrations #4

merged 27 commits into from
May 17, 2017

Conversation

23Skidoo
Copy link
Contributor

@23Skidoo 23Skidoo commented Feb 13, 2017 •
edited
Loading

Plus add a basic test suite. Case 2582.

@jonathanjouty jonathanjouty self-assigned this Feb 13, 2017
@23Skidoo 23Skidoo force-pushed the drop-table-migrations branch 3 times, most recently from f636517 to 20477dd Compare February 13, 2017 15:29
@23Skidoo
Copy link
Contributor Author

Current status: @mariuszrak provided me with some feedback and requests for changes that I'm going to act on in the next few days.

@23Skidoo 23Skidoo assigned 23Skidoo and unassigned mariuszrak Feb 20, 2017
@23Skidoo
Copy link
Contributor Author

Status update: delayed a bit.

@23Skidoo 23Skidoo force-pushed the drop-table-migrations branch 2 times, most recently from a37a4ba to 202130c Compare April 7, 2017 13:08
@23Skidoo
Copy link
Contributor Author

This is done, BTW, just needs CR.

@23Skidoo 23Skidoo assigned mariuszrak and unassigned 23Skidoo and mariuszrak Apr 11, 2017
@mariuszrak
Copy link
Contributor

mariuszrak commented Apr 20, 2017 •
edited by 23Skidoo
Loading

  • I added a commit 0b83f28 with extra unit test. I believe it represents a real scenario that we will hit with, for example, CGI instance. Can we try to adjust branch so scenario described by this test is supported?

  • Can we change name of data MigrationType m to MigrationAction and name of mgrType to mgrAction? I always get confused with things that are names “type” that hold action inside.

  • If we are changing Migration structure - can we change mgrTable :: Table to mgrTable :: RawSQL (). I have a problem with migrations referring a table when I want to remove table definition, but keep migrations.

  • Whole Database.PostgreSQL.PQTypes.Checks module is quite heavy now. It just very hard to follow up on code there - specially inside checkDBConsistency function. I don’t know how to solve it, but maybe you do. Maybe splitting it into smaller files would help?

  • Whole test setup is one big/long test that checks various things. So it’s hard to add new test - because whole setup is one-test-only fight now. Adding a second, even very simple test, would help a lot.

Please don't merge at the end with master - as I will want to adjust kontrakcja and do some migration testing on my machine.

@mariuszrak mariuszrak assigned 23Skidoo and unassigned mariuszrak Apr 20, 2017
mariuszrak
mariuszrak suggested changes Apr 20, 2017
View reviewed changes
Copy link
Contributor

@mariuszrak mariuszrak left a comment •
edited
Loading

Choose a reason for hiding this comment

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

^ Requested changes from previous comment ^

@23Skidoo 23Skidoo assigned mariuszrak and unassigned 23Skidoo May 9, 2017
@23Skidoo
Copy link
Contributor Author

23Skidoo commented May 9, 2017

Implemented the requested changes. The Checks module could use some more refactoring, and the test suite could be extended a little bit, though. I can look into this when I'll have time later this week; otherwise should be ready for re-review.

23Skidoo and others added 18 commits May 12, 2017 11:06
@23Skidoo 23Skidoo requested a review from mariuszrak May 12, 2017 10:42
@mariuszrak
Copy link
Contributor

This looks fine, but I'm doing more work to make this work with kontrakcja on other branch. I'll probably get back to this later this week

@mariuszrak
Review fixes
fc36ee8
@mariuszrak
Merge pull request #6 from scrive/dev-mariusz-drop-tables-with-allow-…
98a5982 
…unknown-tables

checkDatabaseAllowUnknownTables with tests
@mariuszrak mariuszrak merged commit f86d056 into master May 17, 2017
@23Skidoo 23Skidoo deleted the drop-table-migrations branch May 17, 2017 12:57
jonathanjouty
jonathanjouty reviewed May 18, 2017
View reviewed changes
CHANGELOG.md
@@ -0,0 +1,8 @@
# hpqtypes-extras-1.3.0.0 (2017-??-??)
Copy link
Contributor

Choose a reason for hiding this comment

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

@23Skidoo @mariuszrak might want to update this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

@23Skidoo 23Skidoo mentioned this pull request Jan 22, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mariuszrak mariuszrak mariuszrak approved these changes

@jonathanjouty jonathanjouty jonathanjouty left review comments

@arybczak arybczak Awaiting requested review from arybczak

Assignees

@mariuszrak mariuszrak

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@23Skidoo @mariuszrak @jonathanjouty