refactor(core): Delete boilerplate code across migrations (no-changelog) #5254

netroy · 2023-01-25T22:31:16Z

This is part one of streamlining migrations code. Part two will be switching to a DSL that auto-generates the db specific queries from a single code.

This also enables linting on migrations.

DB Tests

flipswitchingmonkey

see note

packages/cli/src/databases/migrations/sqlite/1669739707124-AddWorkflowVersionIdColumn.ts

krynble

Seems ok, tested starting n8n with all databases, all executed fine.

github-actions · 2023-03-27T17:30:05Z

Great PR! Please pay attention to the following items before merging:

Files matching packages/**:

If fixing bug, added test to cover scenario.
If addressing forum or Github issue, added link to description.

Files matching packages/**/*.ts:

Added unit tests to cover new or updated functionality.

Files matching packages/cli/src/databases/migrations/**:

Requested review from at least two engineers on migration.
Avoided irreversible data migrations.
Avoided deleting or updating data keys.
Wrote 'down' migration if possible.

Make sure to check off this list before asking for review.

codecov · 2023-03-29T18:41:42Z

Codecov Report

Patch coverage: 41.17% and no project coverage change.

Comparison is base (0eb4d9f) 20.06% compared to head (090732a) 20.06%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5254   +/-   ##
=======================================
  Coverage   20.06%   20.06%           
=======================================
  Files        2622     2621    -1     
  Lines      118091   118110   +19     
  Branches    18557    18559    +2     
=======================================
+ Hits        23691    23702   +11     
- Misses      93685    93692    +7     
- Partials      715      716    +1

Impacted Files	Coverage Δ
packages/cli/src/databases/config.ts	`73.68% <ø> (ø)`
...ckages/nodes-base/nodes/Kafka/KafkaTrigger.node.ts	`0.00% <0.00%> (ø)`
...ckages/cli/src/databases/utils/migrationHelpers.ts	`62.90% <63.15%> (+4.56%)`	⬆️
packages/cli/src/Db.ts	`56.25% <66.66%> (+1.12%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

ivov

I really like this! Reviewed but untested.

packages/cli/src/Db.ts

packages/cli/src/databases/migrations/mysqldb/1592447867632-WebhookModel.ts

packages/cli/src/databases/migrations/mysqldb/1671726148420-RemoveWorkflowDataLoadedFlag.ts

packages/cli/src/databases/migrations/mysqldb/1675940580449-PurgeInvalidWorkflowConnections.ts

packages/cli/src/databases/types.ts

packages/cli/src/databases/utils/migrationHelpers.ts

packages/cli/src/databases/types.ts

ivov · 2023-03-30T10:56:10Z

packages/cli/src/databases/types.ts

+	tablePrefix: string;
+	dbType: DatabaseType;
+	dbName: string;
+	migrationName: string;


Would it make sense to also add helpers like loadSurveyFromDisk and escapeQuery to the context? Maybe under a utils field?

packages/cli/src/databases/types.ts

packages/cli/src/databases/migrations/mysqldb/1588157391238-InitialMigration.ts

… to not accidentally forget down migrations

ivov

LGTM! I'll test this later today. Some scattered ideas for future:

Any benefit to using the useStructuredResult arg to queryRunner.query()?
Require specifying the reason when choosing to use IrreversibleMigration?
Consolidate usage of escapeQueryWithParameters vs. unescaped queries?
Enforce double quotes on PG table names to avoid casing issues with prefixes?
Require the re-enabling PRAGMA statement after a disabling one on sqlite?
Unify isObjectLiteral which is used in four different spots across packages?

ivov

Tested on sqlite, MySQL 8, PG 13.8 (regular, alt schema, table prefix)

github-actions · 2023-05-05T09:17:07Z

✅ All Cypress E2E specs passed

* master: fix(FTP Node): Use filename instead of remote filepath for downloaded binary data (#6170) ci: Fix test database cleanup (no-changelog) (#6188) refactor(core): Delete boilerplate code across migrations (no-changelog) (#5254) refactor(editor): Add infix to Pinia stores (no-changelog) (#6149) ci: Fix linting issues on master (no-changelog) (#6186)

* master: (31 commits) feat(Date & Time Node): Overhaul of the node (#5904) refactor: Add deprecation notice for WEBHOOK_TUNNEL_URL (#6194) refactor: Add deprecation notice for WEBHOOK_TUNNEL_URL (#6194) feat(HubSpot Node): Overhaul the HubSpot Node (#4337) ci: Create a nightly v1 docker image (no-changelog) (#6197) refactor(core): Add deprecation notice for own mode (#6195) feat(core): Update config defaults for for v1 (no-changelog) (#6196) test(Read Binary Files Node): Unit tests (no-changelog) (#5459) refactor: Add deprecation notice for MySQL and MariaDB (#6189) test: Create custom jest error messages using jest-expect-message (no-changelog) (#5666) fix(core): Move nodeExecute InternalHook calls to hookFunctionsSave (#6193) fix(FTP Node): Use filename instead of remote filepath for downloaded binary data (#6170) ci: Fix test database cleanup (no-changelog) (#6188) refactor(core): Delete boilerplate code across migrations (no-changelog) (#5254) refactor(editor): Add infix to Pinia stores (no-changelog) (#6149) ci: Fix linting issues on master (no-changelog) (#6186) fix(editor): Update and add design system checkbox component to Editor (#6178) fix(editor): Display SSO entry in Settings on Cloud (#6181) feat(Code Node): Add Python support (#4295) fix(editor): Update and fix storybook (was failing to run in local dev mode) (#6180) ... # Conflicts: # packages/editor-ui/src/stores/nodeTypes.store.ts # packages/nodes-base/nodes/Postgres/Postgres.node.ts # packages/nodes-base/nodes/Postgres/v2/actions/versionDescription.ts

…rce-mapper-ui-P2 * feature/resource-mapping-component: (31 commits) feat(Date & Time Node): Overhaul of the node (#5904) refactor: Add deprecation notice for WEBHOOK_TUNNEL_URL (#6194) refactor: Add deprecation notice for WEBHOOK_TUNNEL_URL (#6194) feat(HubSpot Node): Overhaul the HubSpot Node (#4337) ci: Create a nightly v1 docker image (no-changelog) (#6197) refactor(core): Add deprecation notice for own mode (#6195) feat(core): Update config defaults for for v1 (no-changelog) (#6196) test(Read Binary Files Node): Unit tests (no-changelog) (#5459) refactor: Add deprecation notice for MySQL and MariaDB (#6189) test: Create custom jest error messages using jest-expect-message (no-changelog) (#5666) fix(core): Move nodeExecute InternalHook calls to hookFunctionsSave (#6193) fix(FTP Node): Use filename instead of remote filepath for downloaded binary data (#6170) ci: Fix test database cleanup (no-changelog) (#6188) refactor(core): Delete boilerplate code across migrations (no-changelog) (#5254) refactor(editor): Add infix to Pinia stores (no-changelog) (#6149) ci: Fix linting issues on master (no-changelog) (#6186) fix(editor): Update and add design system checkbox component to Editor (#6178) fix(editor): Display SSO entry in Settings on Cloud (#6181) feat(Code Node): Add Python support (#4295) fix(editor): Update and fix storybook (was failing to run in local dev mode) (#6180) ... # Conflicts: # packages/editor-ui/src/stores/nodeTypes.store.ts

janober · 2023-05-25T11:38:33Z

Got released with n8n@0.228.2

netroy added the skip-e2e label Jan 25, 2023

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Jan 25, 2023

netroy force-pushed the cleanup-migrations branch 3 times, most recently from 517a482 to e5118c7 Compare January 30, 2023 12:10

flipswitchingmonkey previously approved these changes Jan 30, 2023

View reviewed changes

packages/cli/src/databases/migrations/sqlite/1669739707124-AddWorkflowVersionIdColumn.ts Outdated Show resolved Hide resolved

netroy dismissed flipswitchingmonkey’s stale review via 27ff255 January 30, 2023 16:49

netroy force-pushed the cleanup-migrations branch from 27ff255 to eb55720 Compare January 30, 2023 17:09

flipswitchingmonkey previously approved these changes Feb 3, 2023

View reviewed changes

netroy dismissed flipswitchingmonkey’s stale review via 93a65c0 February 7, 2023 12:15

netroy force-pushed the cleanup-migrations branch 2 times, most recently from 93a65c0 to 43e8287 Compare February 9, 2023 19:38

netroy requested a review from krynble February 9, 2023 19:39

krynble previously approved these changes Feb 13, 2023

View reviewed changes

netroy dismissed krynble’s stale review via cb1bc4d February 15, 2023 12:09

netroy force-pushed the cleanup-migrations branch from cb1bc4d to 3adfccb Compare February 15, 2023 12:11

netroy force-pushed the cleanup-migrations branch from 3adfccb to aa230f3 Compare March 27, 2023 17:29

netroy requested a review from a team as a code owner March 27, 2023 17:29

refactor(core): Delete boilerplate code across migrations (no-changelog)

ca3be2c

netroy force-pushed the cleanup-migrations branch from aa230f3 to ca3be2c Compare March 29, 2023 18:24

ivov reviewed Mar 30, 2023

View reviewed changes

netroy added 3 commits March 30, 2023 17:14

Merge remote-tracking branch 'upstream/master' into cleanup-migrations

cb36c5e

address some PR comments

c9ed483

Merge remote-tracking branch 'origin/master' into cleanup-migrations

1524fb0

edumaciel10 reviewed Apr 12, 2023

View reviewed changes

packages/cli/src/databases/migrations/mysqldb/1588157391238-InitialMigration.ts Show resolved Hide resolved

This comment was marked as off-topic.

Sign in to view

Merge remote-tracking branch 'origin/master' into cleanup-migrations

6c746c6

netroy added 9 commits April 19, 2023 21:46

update new migrations

a13e451

Merge remote-tracking branch 'origin/master' into cleanup-migrations

ed0f3c2

update new migrations

2df30cd

Merge remote-tracking branch 'origin/master' into cleanup-migrations

4c999a8

lintfix

ff4aeb7

split out migration into reversible and irreversibe types, forcing us…

4746f52

… to not accidentally forget down migrations

optional fields go after mandatory fields

a9d09ca

consolidate migrations.types.ts

ffa5eaf

Merge remote-tracking branch 'origin/master' into cleanup-migrations

a513725

netroy requested a review from ivov May 4, 2023 15:43

allow setting transaction to only false

090732a

netroy force-pushed the cleanup-migrations branch from c94f25b to 090732a Compare May 4, 2023 16:36

ivov reviewed May 5, 2023

View reviewed changes

ivov approved these changes May 5, 2023

View reviewed changes

netroy merged commit 82fe638 into master May 5, 2023

netroy deleted the cleanup-migrations branch May 5, 2023 09:29

janober added the Released label May 25, 2023

netroy mentioned this pull request Jun 28, 2023

fix(core): Ensure valid logger is passed to every migration (no-changelog) #6563

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): Delete boilerplate code across migrations (no-changelog) #5254

refactor(core): Delete boilerplate code across migrations (no-changelog) #5254

netroy commented Jan 25, 2023 •

edited

Loading

flipswitchingmonkey left a comment

krynble left a comment

github-actions bot commented Mar 27, 2023

codecov bot commented Mar 29, 2023 •

edited

Loading

ivov left a comment

ivov Mar 30, 2023

This comment was marked as off-topic.

ivov left a comment

ivov left a comment

github-actions bot commented May 5, 2023

janober commented May 25, 2023

refactor(core): Delete boilerplate code across migrations (no-changelog) #5254

refactor(core): Delete boilerplate code across migrations (no-changelog) #5254

Conversation

netroy commented Jan 25, 2023 • edited Loading

flipswitchingmonkey left a comment

Choose a reason for hiding this comment

krynble left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 27, 2023

codecov bot commented Mar 29, 2023 • edited Loading

Codecov Report

ivov left a comment

Choose a reason for hiding this comment

ivov Mar 30, 2023

Choose a reason for hiding this comment

This comment was marked as off-topic.

ivov left a comment

Choose a reason for hiding this comment

ivov left a comment

Choose a reason for hiding this comment

github-actions bot commented May 5, 2023

janober commented May 25, 2023

netroy commented Jan 25, 2023 •

edited

Loading

codecov bot commented Mar 29, 2023 •

edited

Loading