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
chore: update v1 schema #643
Conversation
Codecov Report
@@ Coverage Diff @@
## main #643 +/- ##
==========================================
+ Coverage 62.07% 64.06% +1.99%
==========================================
Files 113 199 +86
Lines 10755 11861 +1106
Branches 0 87 +87
==========================================
+ Hits 6676 7599 +923
- Misses 3296 3439 +143
- Partials 783 823 +40
Continue to review full report at Codecov.
|
0f51d90
to
bd149b1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's trim down the initial migration as much as we possibly can. The initial schema should be 90% v2, and just supporting v1 in a very soft way.
httpapi.Write(rw, http.StatusNotFound, httpapi.Response{ | ||
Message: fmt.Sprintf("organization %q does not exist", organizationID), | ||
}) | ||
orgID, ok := parseUUID(rw, r, "organization") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why abbreviate here and not for organization
? We should be consistent with abbreviations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's necessarily important for local variables, organization
is a lot to type every time. IMO it's only important for exported variables and function params.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a bit of an arbitrarily drawn line though. It's more to type, but an inconsistent naming scheme hurts readability.
@@ -46,31 +47,37 @@ func List() ([]Example, error) { | |||
returnError = xerrors.Errorf("example %q does not contain README.md", exampleID) | |||
return | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather add a linting rule to enforce newline conventions. Arbitrarily doing it on a per-developer basis will read to inconsistency when reading our code.
9a63ac3
to
2a61e73
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good changes! One minor nit, but that's all.
revoked boolean NOT NULL, | ||
login_type login_type NOT NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These could be removed too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is referenced a bunch in the code, I'd rather remove these after.
2a61e73
to
da23bf5
Compare
Now uses UUIDs for users, organizations, organization members, and api keys. Also updates the v1 migration to correctly create indexes and foreign keys that were missed previously.