Skip to content
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

fix(core): Remove typeorm patches, but still enforce transactions on every migration #6594

Merged
merged 2 commits into from
Jul 5, 2023

Conversation

netroy
Copy link
Member

@netroy netroy commented Jul 4, 2023

This reverts the typeorm patches from #6519

Extracted from #6522

@netroy netroy requested a review from a team as a code owner July 4, 2023 17:02
@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2023

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.

@netroy netroy changed the title revert(core): Remove typeorm patches, but still enforce transactions on every migration fix(core): Remove typeorm patches, but still enforce transactions on every migration Jul 4, 2023
@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Patch coverage: 53.57% and project coverage change: -0.08 ⚠️

Comparison is base (7495e31) 28.91% compared to head (015b734) 28.84%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6594      +/-   ##
==========================================
- Coverage   28.91%   28.84%   -0.08%     
==========================================
  Files        3070     3070              
  Lines      188243   188259      +16     
  Branches    20876    20874       -2     
==========================================
- Hits        54431    54298     -133     
- Misses     132928   133076     +148     
- Partials      884      885       +1     
Impacted Files Coverage Δ
...ckages/cli/src/databases/utils/migrationHelpers.ts 61.53% <53.57%> (-1.37%) ⬇️

... and 5 files with indirect coverage changes

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

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Jul 4, 2023
packages/cli/src/databases/utils/migrationHelpers.ts Outdated Show resolved Hide resolved
export function logMigrationEnd(migrationName: string, disableLogging = inTest): void {
if (disableLogging) return;
function logMigrationEnd(migrationName: string): void {
if (inTest) return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having the messages is always useful, especially if running when testing, don't you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the logic hasn't really changed. we were never passing in a second argument, and disableLogging was always inTest.

@cypress
Copy link

cypress bot commented Jul 5, 2023

2 flaky tests on run #1380 ↗︎

0 228 0 0 Flakiness 2

Details:

🌳 revert-6519 🖥️ browsers:node18.12.0-chrome107 🤖 netroy 🗃️ e2e/*
Project: n8n Commit: 015b734ac6
Status: Passed Duration: 07:32 💡
Started: Jul 5, 2023 9:16 AM Ended: Jul 5, 2023 9:24 AM
Flakiness  18-user-management.cy.ts • 1 flaky test

View Output Video

Test Artifacts
User Management > should delete user and their data Output Screenshots Video
Flakiness  20-workflow-executions.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Current Workflow Executions > should render executions tab correctly Output Screenshots Video

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 5, 2023

✅ All Cypress E2E specs passed

@netroy netroy merged commit 9def7a7 into master Jul 5, 2023
@netroy netroy deleted the revert-6519 branch July 5, 2023 09:26
MiloradFilipovic added a commit that referenced this pull request Jul 5, 2023
* master:
  fix(core): Remove typeorm patches, but still enforce transactions on every migration (#6594)
  feat(Strava Node): Add hide_from_home field in Activity Update (#5883)
  feat(Notion Node): Add option to update icon when updating a page (#5670)
  refactor: Setup node context API, and consolidate code between Webhook and Wait nodes (no-changelog) (#6464)
  fix(core): Fix migrations for MySQL/MariaDB (#6591)
  fix(editor): Show retry information in execution list only when it exists (#6587)
  fix(Strapi Node): Fix issue with pagination (#4991)
  refactor(Item Lists Node): Refactoring (#6575)
  feat: Add crowd.dev node and trigger node (#6082)
  ci: Add Github Action to enforce template to issues (#5295)
  fix(core): Route `/rest/workflows/new` correctly (no-changelog) (#6572)
  fix(Brevo Node): Rename SendInBlue node to Brevo node (#6521)
  fix(XML Node): Fix issue with not returning valid data (#6565)
  feat(HTTP Request Node): New http request generic custom auth credential (#5798)
  fix(core): Fix credentials test (#6569)
  fix(core): Ensure valid `logger` is passed to every migration (no-changelog) (#6563)
  docs: Add irreversible change warning for [email protected] (no-changelog) (#6558)
  feat: Add various source control improvements (#6533)
  feat(Twitter Node): Node overhaul (#4788)
netroy added a commit that referenced this pull request Jul 5, 2023
…every migration (#6594)

* revert(core): Remove typeorm patches, but still enforce transactions on every migration

This reverts #6519

* always re-enable foreign keys, and explicitly rollback transaction
@github-actions github-actions bot mentioned this pull request Jul 5, 2023
This was referenced Jul 5, 2023
@janober
Copy link
Member

janober commented Jul 5, 2023

Got released with [email protected]

netroy added a commit that referenced this pull request Jul 12, 2023
…every migration (#6594)

* revert(core): Remove typeorm patches, but still enforce transactions on every migration

This reverts #6519

* always re-enable foreign keys, and explicitly rollback transaction
@github-actions github-actions bot mentioned this pull request Jul 12, 2023
@github-actions github-actions bot mentioned this pull request Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants