-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[cost center] put all costcenter access behind API #12776
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
757c40c
to
81c2b3f
Compare
started the job as gitpod-build-sefftinge-ubp-model-free-plan-and-12741.3 because the annotations in the pull request description changed |
started the job as gitpod-build-sefftinge-ubp-model-free-plan-and-12741.4 because the annotations in the pull request description changed |
81c2b3f
to
084ddba
Compare
components/gitpod-db/src/typeorm/migration/1662639748206-CostCenterPaymentStrategy.ts
Show resolved
Hide resolved
/hold |
Added the hold for question about the data type. |
/unhold |
|
||
// deleted is restricted for use by db-sync | ||
_ bool `gorm:"column:deleted;type:tinyint;default:0;" json:"deleted"` | ||
LastModified time.Time `gorm:"->:column:_lastModified;type:timestamp;default:CURRENT_TIMESTAMP(6);" json:"_lastModified"` |
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.
out of curiosity: are the gorm definitions manually crafted, or is there a tool to sync them?
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.
manual
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.
There's a tool which can generate them from the DB schema. I used those initially to set up the definitions but often requires some manual tweaking.
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.
Something I actually forgot to mention in yesterday's retro is that I'm getting a little tired of maintaining/writing all the different technical representations (DB & protobuf for TS&Go) of the same shapes. Feels like a lot of code (e.g. this PR) is really just about shoveling data through the stack by copying it into different formats. 😓
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 fair. Codegen can help but it's also important to keep the API shape independent of the shape of the DB. It tends to start very similar, but over time changes. The DB is expected to evolve while the API is expected to retain compatibility.
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.
Yes, I think it would already help if we didn't need to duplicate API to route dashboard requests through server. Also, I think the manual sugar.ts
code could maybe be generated.
public async up(queryRunner: QueryRunner): Promise<void> { | ||
if (!(await columnExists(queryRunner, D_B_COST_CENTER, COL_CREATION_TIME))) { | ||
await queryRunner.query( | ||
`ALTER TABLE ${D_B_COST_CENTER} ADD COLUMN ${COL_CREATION_TIME} varchar(30) NOT NULL, ALGORITHM=INPLACE, LOCK=NONE `, |
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.
they will be filled up with the implicit default value for the varchar
type right? which is ''
, an empty string.
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.
Oh, true. That was not my intend, but I think it will not break.
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'm actually not sure what MYSQL will do with existing records here 😬 The migration worked well against an empty table.
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.
they will be filled up with the implicit default value for the varchar type right? which is '', an empty string.
Just verified that this is the case indeed
Description
This change has unfortunately gotten a little big (still practicing keeping things small 😇).
It does the following:
usage
componentserver
with this APIcreationTime
andbillingStrategy
to cost centerRelated Issue(s)
Fixes #
How to test
You can verify that everything still works by joining the Gitpod team and changing the spending limit
Release Notes
Documentation
Werft options: