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

Big value handling #167

Merged
merged 1 commit into from
May 29, 2020
Merged

Conversation

ajunge
Copy link
Member

@ajunge ajunge commented May 28, 2020

Some claims are very big so a bigger field is needed

Some claims are very big so a bigger field is needed
@simonas-notcat
Copy link
Contributor

@mirceanis this is a potential breaking change for anyone with existing databases. Should we mark it as a breaking change and increment the version nr?

@ajunge
Copy link
Member Author

ajunge commented May 29, 2020

@mirceanis this is a potential breaking change for anyone with existing databases. Should we mark it as a breaking change and increment the version nr?

Why? I think typeorm manage to upgrade the database schema gracefully

@mirceanis
Copy link
Member

I'm not well versed enough in TypeORM to know if this is breaking or not.
How would one check?
I imagine create a daf-based app, use some claims, upgrade daf to this PR, see if everything still works?

If it doesn't work out of the box, then a migration is necessary and if DAF handles that migration, it doesn't count as a breaking change, because the DB schema is internal to DAF.

Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Will merge this in as a breaking change since the schema change is not graceful in all database drivers.

Thanks for the PR

@mirceanis mirceanis merged commit 39a76b6 into decentralized-identity:master May 29, 2020
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.

3 participants