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

Schema fixes and backend #71

Merged
merged 18 commits into from
Apr 5, 2024
Merged

Schema fixes and backend #71

merged 18 commits into from
Apr 5, 2024

Conversation

adi-lux
Copy link
Contributor

@adi-lux adi-lux commented Mar 12, 2024 •
edited by MinhxNguyen7
Loading

Description

Fixing schema issues as well as problems within the initial drizzle migration.

Issues

@adi-lux adi-lux temporarily deployed to staging-71 March 12, 2024 00:37 — with GitHub Actions Inactive
@adi-lux adi-lux self-assigned this Mar 12, 2024
@adi-lux adi-lux added the codefix Improvements to the codebase label Mar 12, 2024
@adi-lux adi-lux linked an issue Mar 12, 2024 that may be closed by this pull request
Drizzle migration fixes #68
Closed
@adi-lux adi-lux temporarily deployed to staging-71 March 14, 2024 07:17 — with GitHub Actions Inactive
@adi-lux adi-lux temporarily deployed to staging-71 March 14, 2024 22:51 — with GitHub Actions Inactive
KevinWu098
KevinWu098 reviewed Mar 14, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
@KevinWu098 KevinWu098 mentioned this pull request Mar 27, 2024
Merged
5 tasks
@adi-lux adi-lux temporarily deployed to staging-71 March 27, 2024 23:41 — with GitHub Actions Inactive
MinhxNguyen7
MinhxNguyen7 requested changes Mar 28, 2024
View reviewed changes
Copy link
Member

@MinhxNguyen7 MinhxNguyen7 left a comment

Choose a reason for hiding this comment

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

Do we need the snapshots and migrations?

Also, @adi-lux and @KevinWu098, can you decide on the location for migrations. because it looks like your PRs disagree on that.

src/lib/db/schema.ts Show resolved Hide resolved
@adi-lux
Copy link
Contributor Author

adi-lux commented Apr 3, 2024

Do we need the snapshots and migrations?

Also, @adi-lux and @KevinWu098, can you decide on the location for migrations. because it looks like your PRs disagree on that.

Maybe we could just remove the snapshots and migrations for now and readd them before a 1.0 release? I think putting the migrations inside the db folder works, but I'm fine with either

@MinhxNguyen7
Copy link
Member

Do we need the snapshots and migrations?
Also, @adi-lux and @KevinWu098, can you decide on the location for migrations. because it looks like your PRs disagree on that.

Maybe we could just remove the snapshots and migrations for now and readd them before a 1.0 release? I think putting the migrations inside the db folder works, but I'm fine with either

Yeah that's fine.

@MinhxNguyen7
Copy link
Member

MinhxNguyen7 commented Apr 3, 2024 •
edited
Loading

Also, is this ready for review? If so, can you resolve the merge conflicts?

@adi-lux adi-lux temporarily deployed to staging-71 April 5, 2024 00:55 — with GitHub Actions Inactive
@adi-lux adi-lux marked this pull request as ready for review April 5, 2024 00:55
@adi-lux
Copy link
Contributor Author

adi-lux commented Apr 5, 2024 •
edited
Loading

Also, is this ready for review? If so, can you resolve the merge conflicts?

Fixed everything, should be good to merge. I think the deploy to staging thing fails because there is no actual database url to migrate to.

KevinWu098
KevinWu098 previously requested changes Apr 5, 2024
View reviewed changes
Copy link
Member

@KevinWu098 KevinWu098 left a comment

Choose a reason for hiding this comment

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

A few more non-blocking comments on one-line fixes

src/lib/db/schema.ts Outdated Show resolved Hide resolved
src/routes/summary/+page.server.ts Outdated Show resolved Hide resolved
@MinhxNguyen7
Copy link
Member

Also, is this ready for review? If so, can you resolve the merge conflicts?

Fixed everything, should be good to merge. I think the deploy to staging thing fails because there is no actual database url to migrate to.

Are you sure? The deployment worked until you merged main in. Otherwise, I'm good to merge it

@adi-lux adi-lux temporarily deployed to staging-71 April 5, 2024 20:22 — with GitHub Actions Inactive
@adi-lux
chore: 🔧 remove unused page
97af4e4 
Signed-off-by: Adithya Anandsaikrishnan <[email protected]>
@adi-lux adi-lux temporarily deployed to staging-71 April 5, 2024 20:23 — with GitHub Actions Inactive
@adi-lux adi-lux temporarily deployed to staging-71 April 5, 2024 20:27 — with GitHub Actions Inactive
@adi-lux
Copy link
Contributor Author

adi-lux commented Apr 5, 2024

Also, is this ready for review? If so, can you resolve the merge conflicts?

Fixed everything, should be good to merge. I think the deploy to staging thing fails because there is no actual database url to migrate to.

Are you sure? The deployment worked until you merged main in. Otherwise, I'm good to merge it

Finally fixed it, turns out the problem was in the postinstall script and not the DB url.

MinhxNguyen7
MinhxNguyen7 approved these changes Apr 5, 2024
View reviewed changes
Copy link
Member

@MinhxNguyen7 MinhxNguyen7 left a comment

Choose a reason for hiding this comment

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

Seems alright

@MinhxNguyen7 MinhxNguyen7 dismissed KevinWu098’s stale review April 5, 2024 21:22

Changes addressed

@MinhxNguyen7 MinhxNguyen7 merged commit bf6d7b3 into main Apr 5, 2024
2 checks passed
@KevinWu098 KevinWu098 deleted the aa/schema-fixes-and-backend branch October 17, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MinhxNguyen7 MinhxNguyen7 MinhxNguyen7 approved these changes

@KevinWu098 KevinWu098 KevinWu098 left review comments

Assignees

@adi-lux adi-lux

Labels
codefix Improvements to the codebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Drizzle migration fixes Guest user support
3 participants
@adi-lux @MinhxNguyen7 @KevinWu098