refactor(core): Abstract away InstanceSettings and `encryptionKey` into injectable services (no-changelog) #7471

netroy · 2023-10-20T07:08:17Z

This change ensures that things like encryptionKey and instanceId are always available directly where they are needed, instead of passing them around throughout the code.

DB Tests
E2E Tests

github-actions · 2023-10-20T07:08:41Z

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.

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

codecov · 2023-10-20T07:29:50Z

Codecov Report

Attention: 98 lines in your changes are missing coverage. Please review.

Comparison is base (519680c) 33.55% compared to head (7e6ebf9) 33.55%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7471      +/-   ##
==========================================
- Coverage   33.55%   33.55%   -0.01%     
==========================================
  Files        3398     3399       +1     
  Lines      207597   207444     -153     
  Branches    22428    22414      -14     
==========================================
- Hits        69652    69600      -52     
+ Misses     136823   136716     -107     
- Partials     1122     1128       +6

Files	Coverage Δ
packages/cli/src/AbstractServer.ts	`48.51% <ø> (-0.51%)`	⬇️
packages/cli/src/CrashJournal.ts	`54.16% <100.00%> (+4.16%)`	⬆️
...i/src/ExternalSecrets/ExternalSecretsManager.ee.ts	`71.87% <100.00%> (-0.95%)`	⬇️
packages/cli/src/Ldap/helpers.ts	`83.63% <100.00%> (-0.67%)`	⬇️
packages/cli/src/Mfa/totp.service.ts	`90.90% <100.00%> (+0.90%)`	⬆️
...Api/v1/handlers/credentials/credentials.handler.ts	`97.05% <100.00%> (ø)`
.../v1/handlers/credentials/credentials.middleware.ts	`95.00% <100.00%> (ø)`
...Api/v1/handlers/credentials/credentials.service.ts	`68.75% <100.00%> (-0.39%)`	⬇️
packages/cli/src/WorkflowExecuteAdditionalData.ts	`12.94% <100.00%> (ø)`
packages/cli/src/audit/risks/instance.risk.ts	`90.66% <100.00%> (+0.12%)`	⬆️
... and 46 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…to injectable services

netroy · 2023-10-20T15:19:39Z

packages/cli/src/PublicApi/v1/handlers/credentials/credentials.service.ts

@@ -87,8 +87,6 @@ export async function removeCredential(credentials: CredentialsEntity): Promise<
 }

 export async function encryptCredential(credential: CredentialsEntity): Promise<ICredentialsDb> {


we should setup the require-await linting rule to get rid of unnecessary async markers like these.

packages/cli/jest.config.js

packages/cli/src/CredentialsHelper.ts

packages/cli/src/InternalHooks.ts

packages/cli/src/License.ts

packages/core/src/NodeExecuteFunctions.ts

packages/core/test/helpers/index.ts

packages/node-dev/commands/build.ts

packages/workflow/test/Helpers.ts

Co-authored-by: Iván Ovejero <ivov.src@gmail.com>

ivov

Thanks for addressing everything! Only minor comments, feel free to merge.

ivov · 2023-10-23T08:13:33Z

packages/cli/src/CrashJournal.ts

@@ -16,7 +17,8 @@ export const touchFile = async (filePath: string): Promise<void> => {
 	}
 };

-const journalFile = join(UserSettings.getUserN8nFolderPath(), 'crash.journal');
+const { n8nFolder } = Container.get(InstanceSettings);
+const journalFile = join(n8nFolder, 'crash.journal');


We could move the filepath to InstanceSettings?

ivov · 2023-10-23T08:23:06Z

packages/cli/src/config/schema.ts

@@ -881,7 +882,7 @@ export const schema = {
 			location: {
 				doc: 'Log file location; only used if log output is set to file.',
 				format: String,
-				default: path.join(UserSettings.getUserN8nFolderPath(), 'logs/n8n.log'),
+				default: path.join(Container.get(InstanceSettings).n8nFolder, 'logs/n8n.log'),


Or move to InstanceSettings? Not 100% sure where the line should be though, but I like the thought of having this centralized.

we can revisit this later. Maybe there could be another file in cli that manages all n8n subfolders.

ivov · 2023-10-23T08:23:26Z

packages/cli/src/config/schema.ts

@@ -947,7 +948,7 @@ export const schema = {
 		},
 		localStoragePath: {
 			format: String,
-			default: path.join(UserSettings.getUserN8nFolderPath(), 'binaryData'),
+			default: path.join(Container.get(InstanceSettings).n8nFolder, 'binaryData'),


Same as above.

ivov · 2023-10-23T08:23:39Z

packages/cli/src/constants.ts

+export const GENERATED_STATIC_DIR = join(
+	Container.get(InstanceSettings).userHome,
+	'.cache/n8n/public',
+);


Same as above.

ivov · 2023-10-23T08:25:25Z

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

+		const filename = `${
+			Container.get(InstanceSettings).n8nFolder
+		}/${PERSONALIZATION_SURVEY_FILENAME}`;


Same as above.

packages/cli/test/unit/ExternalSecrets/ExternalSecretsManager.test.ts

github-actions · 2023-10-23T09:01:58Z

⚠️ Some Cypress E2E specs are failing, please fix them before merging

cypress · 2023-10-23T09:02:05Z

1 flaky test on run #2545 ↗︎

0	258	0	0	1

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 netroy 🗃️ e2e/*
Project: n8n	Commit: `7e6ebf9883`
Status: Passed	Duration: 08:33 💡
Started: Oct 23, 2023 11:30 AM	Ended: Oct 23, 2023 11:39 AM

cypress/e2e/2-credentials.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
Credentials > should create a new credential using Add Credential button		`Output` `Screenshots` `Video`

Review all test suite changes for PR #7471 ↗︎

will revisit in another PR

github-actions · 2023-10-23T11:39:10Z

✅ All Cypress E2E specs passed

…nglog) (#7501) This was the expected behavior, until I changed it in #7471

* master: (30 commits) 🚀 Release 1.14.0 (#7514) ci: Fix oclif manifest generation feat(Switch Node): Add support for infinite Switch outputs (#7499) 🚀 Release 1.13.0 (#7512) fix(core): Ensure nodes post-processors run in the correct order (#7500) feat(editor): Add PH tracking to event (#7511) fix(core): Fix workflow activation with history and workflow history for EE (no-changelog) (#7508) refactor(core): Make executions pruning more resilient (#7480) fix(Spreadsheet File Node): Fix include empty cells not working with v2 (#7505) fix(core): Create instance settings directory recursively (no-changelog) (#7506) fix(Microsoft SQL Node): Prevent SQL injection (#7467) refactor(core): Make pruning via lifecycle configuration in S3 mode mandatory (#7482) fix(core): Always derive `instanceId` from the encryption key (no-changlog) (#7501) fix(MQTT Trigger Node): Fix node causing a start up hang when active (#7498) feat: Collect usage metrics on license renewal (no-changelog) (#7486) fix(core): Fix `frontend.settings` external hook execution (#7496) fix(Redis Node): Fix adding sets data types (#7444) fix: Save new version of the workflow instead of the previous (no-changelog) (#7428) refactor(core): Abstract away InstanceSettings and `encryptionKey` into injectable services (no-changelog) (#7471) fix(Customer.io Node): Fix api endpoint when using EU region (#7485) ...

janober · 2023-10-25T14:31:34Z

Got released with n8n@1.14.0

netroy requested a review from a team as a code owner October 20, 2023 07:08

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

refactor(core): Abstract away InstanceSettings and encryptionKey in…

510284c

…to injectable services

netroy force-pushed the InstanceSettings branch from c2d68f7 to 510284c Compare October 20, 2023 13:52

netroy commented Oct 20, 2023

View reviewed changes

ivov reviewed Oct 20, 2023

View reviewed changes

netroy changed the title ~~refactor(core): Abstract away InstanceSettings and encryptionKey into injectable services~~ refactor(core): Abstract away InstanceSettings and encryptionKey into injectable services (no-changelog) Oct 20, 2023

Apply suggestions from code review

9f251ec

Co-authored-by: Iván Ovejero <ivov.src@gmail.com>

netroy force-pushed the InstanceSettings branch from de02993 to 9f251ec Compare October 20, 2023 17:21

netroy requested a review from ivov October 20, 2023 17:24

ivov previously approved these changes Oct 23, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into InstanceSettings

4ea8662

revert InternalHooks.init

6d40dc8

will revisit in another PR

netroy dismissed ivov’s stale review via 6d40dc8 October 23, 2023 09:03

netroy added 2 commits October 23, 2023 11:45

Merge remote-tracking branch 'origin/master' into InstanceSettings

287d71e

Update ExternalSecretsManager.test.ts

7e6ebf9

netroy requested a review from ivov October 23, 2023 09:56

ivov approved these changes Oct 23, 2023

View reviewed changes

netroy merged commit b6de910 into master Oct 23, 2023
61 of 62 checks passed

netroy deleted the InstanceSettings branch October 23, 2023 11:39

netroy mentioned this pull request Oct 23, 2023

fix(core): Always derive instanceId from the encryption key (no-changlog) #7501

Merged

netroy added a commit that referenced this pull request Oct 24, 2023

fix(core): Always derive instanceId from the encryption key (no-cha…

a9fdd01

…nglog) (#7501) This was the expected behavior, until I changed it in #7471

janober added the Released label Oct 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): Abstract away InstanceSettings and `encryptionKey` into injectable services (no-changelog) #7471

refactor(core): Abstract away InstanceSettings and `encryptionKey` into injectable services (no-changelog) #7471

netroy commented Oct 20, 2023

github-actions bot commented Oct 20, 2023

codecov bot commented Oct 20, 2023 •

edited

Loading

netroy Oct 20, 2023

ivov left a comment

ivov Oct 23, 2023

ivov Oct 23, 2023

netroy Oct 23, 2023

ivov Oct 23, 2023

ivov Oct 23, 2023

ivov Oct 23, 2023

github-actions bot commented Oct 23, 2023

cypress bot commented Oct 23, 2023 •

edited

Loading

github-actions bot commented Oct 23, 2023

janober commented Oct 25, 2023

		@@ -87,8 +87,6 @@ export async function removeCredential(credentials: CredentialsEntity): Promise<
		}

		export async function encryptCredential(credential: CredentialsEntity): Promise<ICredentialsDb> {

refactor(core): Abstract away InstanceSettings and encryptionKey into injectable services (no-changelog) #7471

refactor(core): Abstract away InstanceSettings and encryptionKey into injectable services (no-changelog) #7471

Conversation

netroy commented Oct 20, 2023

github-actions bot commented Oct 20, 2023

codecov bot commented Oct 20, 2023 • edited Loading

Codecov Report

netroy Oct 20, 2023

Choose a reason for hiding this comment

ivov left a comment

Choose a reason for hiding this comment

ivov Oct 23, 2023

Choose a reason for hiding this comment

ivov Oct 23, 2023

Choose a reason for hiding this comment

netroy Oct 23, 2023

Choose a reason for hiding this comment

ivov Oct 23, 2023

Choose a reason for hiding this comment

ivov Oct 23, 2023

Choose a reason for hiding this comment

ivov Oct 23, 2023

Choose a reason for hiding this comment

github-actions bot commented Oct 23, 2023

cypress bot commented Oct 23, 2023 • edited Loading

1 flaky test on run #2545 ↗︎

Review all test suite changes for PR #7471 ↗︎

github-actions bot commented Oct 23, 2023

janober commented Oct 25, 2023

refactor(core): Abstract away InstanceSettings and `encryptionKey` into injectable services (no-changelog) #7471

refactor(core): Abstract away InstanceSettings and `encryptionKey` into injectable services (no-changelog) #7471

codecov bot commented Oct 20, 2023 •

edited

Loading

cypress bot commented Oct 23, 2023 •

edited

Loading