-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[db][usage] Add d_b_billed_session table migration and Go definitions #11868
Conversation
feb29dd
to
fd6cec3
Compare
15b968b
to
e04ef72
Compare
teamID := uuid.New().String() | ||
teamAttributionID := db.NewTeamAttributionID(teamID) | ||
|
||
instance := dbtest.NewWorkspaceInstance(t, db.WorkspaceInstance{ |
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 do we create a WorkspaceInstance record here?
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.
Now that you ask it I realize it's actually unnecessary 😬 Will remove it, thanks!
InvoiceID: "some-invoice-ID", | ||
} | ||
|
||
tx := conn.Create(billedSession) |
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.
You should also delete the created record at the end of the test. For other DB models, there are helpers (in dbtest
package) that do this automatically by hooking into the t *testing.T
when the test finishes.
export class BilledSession1659601647550 implements MigrationInterface { | ||
public async up(queryRunner: QueryRunner): Promise<void> { | ||
await queryRunner.query( | ||
"CREATE TABLE IF NOT EXISTS `d_b_billed_session` (`instanceId` char(36) NOT NULL, `from` varchar(255) NOT NULL, `to` varchar(255) NOT NULL DEFAULT '', `system` varchar(255) NOT NULL, `invoiceId` varchar(255) NOT NULL DEFAULT '', `deleted` tinyint(4) NOT NULL DEFAULT '0', `_lastModified` timestamp(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6), PRIMARY KEY (`instanceId`, `from`), KEY `ind_dbsync` (`_lastModified`)) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;", |
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.
Can we specify an enum type, so that we can constrain system
to be either "stripe" or "chargebee"?
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 think keeping the column as string, and the in code ensuring an Enum is slightly more extensible for the future.
|
||
import { MigrationInterface, QueryRunner } from "typeorm"; | ||
|
||
export class BilledSession1659601647550 implements MigrationInterface { |
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.
How did you generate this migration? My own process for generating a migration for a new table using typeorm is somewhat convoluted 😬
I think there is an opportunity to improve the README here.
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.
Ohh, it's this README instead of the one you linked above. Definitely an opportunity to improve how we point to the right docs 🙈
/hold |
Co-authored-by: Laurie T. Malau <laurie@gitpod.io>
e431138
to
3533c3f
Compare
Description
Add d_b_billed_session table migration and Go definitions
Related Issue(s)
Fixes #11867
How to test
In
/components/usage/pkg/db
rungo test
Release Notes
Documentation
Werft options: