-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[usage] Add db.Project model in golang #10368
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
Conversation
c884749
to
47991a8
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.
Many thanks for this PR! Left a few questions in-line for now & will finish this code review later today.
components/usage/pkg/db/project.go
Outdated
TeamID sql.NullString `gorm:"column:teamId;type:char;size:36;" json:"teamId"` | ||
UserID sql.NullString `gorm:"column:userId;type:char;size:36;" json:"userId"` | ||
|
||
MarkedDeleted int32 `gorm:"column:markedDeleted;type:tinyint;default:0;" json:"markedDeleted"` |
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.
Question: Why use int32
to represent booleans?
- Shouldn't this be a
bool
? - SQL's
tinyint
is one byte, so this could also beuint8
orbyte
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.
The underlying datatype in MySQL is tinyint it looks like, so here it only matches that. I'll see if using a bool results in similar behaviour.
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've updated this to bool. It works fine and gives us nicer handling with the model. Great spot!
47991a8
to
5b940b2
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.
Many thanks for adjusting! 👍
A few more comments in-line.
Also, how do you run the unit tests?
|
||
SoftDeleted sql.NullString `gorm:"column:softDeleted;type:char;size:4;" json:"softDeleted"` | ||
Pinned int32 `gorm:"column:pinned;type:tinyint;default:0;" json:"pinned"` | ||
Pinned bool `gorm:"column:pinned;type:tinyint;default:0;" json:"pinned"` | ||
|
||
// deleted is reserved for use by db-sync | ||
_ int32 `gorm:"column:deleted;type:tinyint;default:0;" json:"deleted"` |
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.
_ int32 `gorm:"column:deleted;type:tinyint;default:0;" json:"deleted"` | |
_ bool `gorm:"column:deleted;type:tinyint;default:0;" json:"deleted"` |
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.
Also,
Workspace.ID
andWorkspace.OwnerID
could be of typeuuid.UUID
Workspace.Type
could be an enum [1]
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 spot. I'd like to leave this part for later PRs once we start using the data from the model and come across this. This will guide us exactly how we want to encapsulated the enum model for this.
Co-authored-by: Jan Keromnes <[email protected]>
Standard go test. Navigate to the component and do |
Thanks! I took the liberty to clarify the "How to test" instructions in your PR description. 😇 Also, could these eventually be run by Werft, so that we automatically detect new bugs / regressions? 👀 |
I tried this -- did I do something wrong?
|
@jankeromnes see instructions in the error message.
|
The tests are already run by werft :) See raw logs and search for |
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.
Thanks again for helping figure out the test instructions!
All nits are addressed, the code looks good, and it works as advertised. 👍
LGTM* 🐐
*(LGTM = Let Goats Try Minecraft)
<3 |
Description
Adds golang model for
d_b_project
table. Process similar to #10293Related Issue(s)
How to test
Unit tests
Release Notes
Documentation
NONE