⬇️ Merge release v5.2 to dev branch with typeorm syntax updates for feature flag #1878

ppratikcr7 · 2024-08-29T06:39:54Z

Context

This PR is to resolve #1875. Here we want to pull the typeORM v0.3.0 update related changes to dev branch to release v6.0 with feature flags.

Summary

Here I have made a new branch release/v6.0 from dev branch and pulled in release/v5.2 branch. Updated typeorm syntax for feature flag related code.

Key Changes

Updated typeORM syntax for feature flag related code in dev branch.
In typeORM v0.3.0 { primary: true } option to define a column as primary key is not supported for relational columns like @ManyToMany, @OneToMany. We need to define separate @PrimaryColumn and use @JoinColum with the relational column. This is done in this PR for feature flag related models.
In the typeORMLoader.ts file, we need to add any new entities/models that we create and also add any new migrations class.
Created a migration file for the model changes made.
Updated migration commands as with typeorm update the old command seems to generate migration files in wrong directory.

(cherry picked from commit b22767d)

bcb37 · 2024-08-29T18:35:32Z

In the typeORMLoader.ts file, we need to add any new entities/models that we create and also add any new migrations class.

That's really annoying. Why can't Typeorm keep track of and discover these things anymore? I know I'll forget to do this step a lot.

bcb37 · 2024-08-29T19:24:55Z

backend/packages/Upgrade/src/loaders/typeormLoader.ts

+import { removeGroupIdFromIndividualExclusion1721124249413 } from '../database/migrations/1721124249413-removeGroupIdFromIndividualExclusion';
+import { featureFlagFilterModeChangeDefault1722540825048 } from '../database/migrations/1722540825048-featureFlagFilterModeChangeDefault';
+
+const entities = [


Could we not just use env.app.dirs.entities instead of having to define this and update it all the time? I tested it locally and it works.

bcb37 · 2024-08-29T19:49:26Z

backend/packages/Upgrade/src/loaders/typeormLoader.ts

+  UserStratificationFactor,
+];
+
+export const migrations = [


Same here. I was able to use env.app.dirs.migrations and did not have to define this variable.

bcb37 · 2024-08-29T19:50:51Z

backend/packages/Upgrade/test/utils/database.ts

-    migrations: env.app.dirs.migrations,
-  });
-  return connection;
+    migrations: migrations,


I successfully ran tests using env.app.dirs.migration instead of importing the variable and it worked fine.

It's deprecated and will be removed from the 0.4.0 version. Check the changelog Breaking Change

Lets go with @bcb37 suggestion till they force us to upgrade :)

bcb37 · 2024-08-29T19:50:59Z

backend/packages/Upgrade/test/utils/database.ts

+    password: env.db.password,
+    logging: env.db.logging as LoggerOptions,
+    entities: env.app.dirs.entities,
+    migrations: migrations,


bcb37 · 2024-08-29T19:51:53Z

backend/packages/Upgrade/package.json

-        "migration:run": "./node_modules/.bin/ts-node -r tsconfig-paths/register ./node_modules/.bin/typeorm migration:run",
+        "migration:generate": "./node_modules/.bin/ts-node -r tsconfig-paths/register ./node_modules/.bin/typeorm migration:generate -d src/loaders/typeormLoader.ts",
+        "migration:create": "./node_modules/.bin/ts-node -r tsconfig-paths/register ./node_modules/.bin/typeorm migration:create -d src/loaders/typeormLoader.ts",
+        "migration:run": "./node_modules/.bin/ts-node -r tsconfig-paths/register ./node_modules/.bin/typeorm migration:run -d src/loaders/typeormLoader.ts",
        "migration:revert": "./node_modules/.bin/ts-node -r tsconfig-paths/register ./node_modules/.bin/typeorm migration:revert"


We should add the -d flag to the revert script here too

bcb37

A few things I noticed, which were probably in the original pr.
Also, should the migration in the top level 1724745446870-typeormv3-featureFlagUpdates.ts be moved?

backend/packages/Upgrade/src/api/repositories/AnalyticsRepository.ts

VivekFitkariwala · 2024-08-30T07:51:09Z

backend/packages/Upgrade/1724745446870-typeormv3-featureFlagUpdates.ts

@@ -0,0 +1,50 @@
+import { MigrationInterface, QueryRunner } from "typeorm";


Is this file needed?

Yes we need this, somehow the file was created in the Upgrade directory due to recent changes to migration script. Will look into this.

ppratikcr7 · 2024-08-30T09:50:25Z

In the typeORMLoader.ts file, we need to add any new entities/models that we create and also add any new migrations class.

That's really annoying. Why can't Typeorm keep track of and discover these things anymore? I know I'll forget to do this step a lot.

Right, I was also feeling the same as it was done in the old PR for typeorm changes and I was going to look for a solution. Yes, indeed env.app.dirs.entities and env.app.dirs.migration works fine. Thanks :)

…n and entities

ppratikcr7 · 2024-09-03T12:24:16Z

@bcb37 @VivekFitkariwala Made changes as requested to remove excess changes to the datasource typeLoader file related to annoying updates to the entities and migration on each new addition. We can update once its forced in typeorm v0.4.

Also, checked the migration scripts and resolved them to generate migration files in proper directory src/database/migrations. The change required to run the migration script is as below for the generate and create cli commands (we need to pass the migration file name as an argument name):

name=migration-name npm run migration:generate
name=migration-name npm run migration:create

The other two migration scripts for run and revert should remain the same:

npm run migration:run
npm run migration:revert

Also, I had to update the typeormloader to have a default export of the datasource due to following error after removing the entities and migrations export from it:

Error: Given data source file must contain export of a DataSource instance

ppratikcr7 · 2024-09-03T13:13:41Z

Once, this is approved, will add these changes in the slack and gitbook documentation.

…into feature-flags/merge-typeorm-v0.3-updates

resolved

Merge release v5.2 to dev branch with typeorm syntax updates for ff

e9b3832

(cherry picked from commit b22767d)

ppratikcr7 requested review from VivekFitkariwala and bcb37 August 29, 2024 06:39

ppratikcr7 self-assigned this Aug 29, 2024

ppratikcr7 changed the title ~~⬇️ Merge release v5.2 to dev branch with typeorm syntax updates for feature flag #1876~~ ⬇️ Merge release v5.2 to dev branch with typeorm syntax updates for feature flag Aug 29, 2024

ppratikcr7 added this to the Program Increment PI12 milestone Aug 29, 2024

ppratikcr7 added the priority: high High priority issue label Aug 29, 2024

bcb37 reviewed Aug 29, 2024

View reviewed changes

bcb37 previously requested changes Aug 29, 2024

View reviewed changes

VivekFitkariwala requested changes Aug 30, 2024

View reviewed changes

updates to datasource typeloader to remove excess changes to migratio…

e278544

…n and entities

VivekFitkariwala previously approved these changes Sep 3, 2024

View reviewed changes

Merge branch 'dev' of https://github.com/CarnegieLearningWeb/UpGrade …

b2012d7

…into feature-flags/merge-typeorm-v0.3-updates

ppratikcr7 dismissed VivekFitkariwala’s stale review via b2012d7 September 4, 2024 06:59

Merge branch 'dev' of https://github.com/CarnegieLearningWeb/UpGrade …

de0ffed

…into feature-flags/merge-typeorm-v0.3-updates

ppratikcr7 requested review from VivekFitkariwala and bcb37 September 4, 2024 07:00

fix for feature flag failed test case due to typeorm changes

26ecb81

VivekFitkariwala approved these changes Sep 4, 2024

View reviewed changes

danoswaltCL merged commit 154c558 into dev Sep 4, 2024
12 checks passed

danoswaltCL deleted the feature-flags/merge-typeorm-v0.3-updates branch September 4, 2024 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⬇️ Merge release v5.2 to dev branch with typeorm syntax updates for feature flag #1878

⬇️ Merge release v5.2 to dev branch with typeorm syntax updates for feature flag #1878

ppratikcr7 commented Aug 29, 2024 •

edited

Loading

bcb37 commented Aug 29, 2024

bcb37 Aug 29, 2024

bcb37 Aug 29, 2024

bcb37 Aug 29, 2024

VivekFitkariwala Aug 30, 2024 •

edited

Loading

VivekFitkariwala Aug 30, 2024

bcb37 Aug 29, 2024

bcb37 Aug 29, 2024

bcb37 left a comment

VivekFitkariwala Aug 30, 2024

ppratikcr7 Aug 30, 2024 •

edited

Loading

ppratikcr7 commented Aug 30, 2024

ppratikcr7 commented Sep 3, 2024 •

edited

Loading

ppratikcr7 commented Sep 3, 2024

		@@ -0,0 +1,50 @@
		import { MigrationInterface, QueryRunner } from "typeorm";

⬇️ Merge release v5.2 to dev branch with typeorm syntax updates for feature flag #1878

⬇️ Merge release v5.2 to dev branch with typeorm syntax updates for feature flag #1878

Conversation

ppratikcr7 commented Aug 29, 2024 • edited Loading

Context

Summary

Key Changes

bcb37 commented Aug 29, 2024

bcb37 Aug 29, 2024

Choose a reason for hiding this comment

bcb37 Aug 29, 2024

Choose a reason for hiding this comment

bcb37 Aug 29, 2024

Choose a reason for hiding this comment

VivekFitkariwala Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

VivekFitkariwala Aug 30, 2024

Choose a reason for hiding this comment

bcb37 Aug 29, 2024

Choose a reason for hiding this comment

bcb37 Aug 29, 2024

Choose a reason for hiding this comment

bcb37 left a comment

Choose a reason for hiding this comment

VivekFitkariwala Aug 30, 2024

Choose a reason for hiding this comment

ppratikcr7 Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

ppratikcr7 commented Aug 30, 2024

ppratikcr7 commented Sep 3, 2024 • edited Loading

ppratikcr7 commented Sep 3, 2024

ppratikcr7 commented Aug 29, 2024 •

edited

Loading

VivekFitkariwala Aug 30, 2024 •

edited

Loading

ppratikcr7 Aug 30, 2024 •

edited

Loading

ppratikcr7 commented Sep 3, 2024 •

edited

Loading