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

feat: schema migration #598

Merged
merged 21 commits into from
Apr 23, 2023
Merged

Conversation

TimoKramer
Copy link
Member

@TimoKramer TimoKramer commented Jan 23, 2023

SUMMARY

Checks

Feature
  • Implements an existing feature request. Make sure the feature request has been accepted for implementation before opening a PR.
  • Related issues linked using fixes #number
  • Documentation added
  • Formatting checked

@TimoKramer TimoKramer self-assigned this Jan 23, 2023
@TimoKramer TimoKramer marked this pull request as draft January 23, 2023 09:57
@TimoKramer
Copy link
Member Author

@fpischedda I am thinking since you wished for a way to avoid conflicting schema migrations with colleagues that we could have something like a tool that tracks the migrations in a separate file. Then you would get merge conflicts in case you didn't rebase the last commit to this tracking file. @kordano came up with the idea, there is a tool called atlas for sql-dbs that does this already.

@fpischedda
Copy link

Hi @TimoKramer

This is a neat idea! I will also have a look at atlas, it could be useful at $work, thanks @kordano for mentioning it 🙂

@TimoKramer TimoKramer marked this pull request as ready for review February 10, 2023 16:47
doc/schema-migration.md Outdated Show resolved Hide resolved
resources/DATAHIKE_VERSION Outdated Show resolved Hide resolved
@jsmassa
Copy link
Collaborator

jsmassa commented Feb 27, 2023

I think there is something missing in the documentation that describes in which cases it makes sense to use norms for schema migration. For someone that hasn't read the conversation in this issue it doesn't become quite clear why you would need to have norms installed for something that would need to be done only once, naively assuming you only have one database of a kind.

Copy link
Member

@whilo whilo left a comment

Choose a reason for hiding this comment

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

Overall looks good, but I will still need to take a closer look after the clarifications.

doc/schema-migration.md Show resolved Hide resolved
doc/schema-migration.md Show resolved Hide resolved
doc/schema-migration.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@jsmassa jsmassa left a comment

Choose a reason for hiding this comment

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

I like it much better now. I think the "Why using the schema migration tool?" section prevents the confusion I felt in the first version for a new reader.

The resources/.touch file slipped in again though and can be removed.

bb/resources/native-image-tests/run-normtests Outdated Show resolved Hide resolved
doc/schema-migration.md Outdated Show resolved Hide resolved
Copy link
Member

@kordano kordano left a comment

Choose a reason for hiding this comment

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

There is still an empty resources/.touchin there.

@@ -0,0 +1,120 @@
(ns datahike.norm.norm-test
Copy link
Member

Choose a reason for hiding this comment

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

Very nice tests. Can you think of any edge cases where the norms might break?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I am not aware of any right now.

whilo
whilo previously approved these changes Mar 16, 2023
@whilo whilo dismissed their stale review March 16, 2023 02:08

I leave it to the other reviewers to have the final word on this.

@TimoKramer TimoKramer merged commit 11da6f1 into replikativ:main Apr 23, 2023
@TimoKramer TimoKramer deleted the feat/13-schema_migration branch April 23, 2023 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Schema Migration
5 participants