-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
refactor(core): Move instanceType
to InstanceSettings
(no-changelog)
#10640
Conversation
packages/cli/src/services/orchestration/webhook/handle-command-message-webhook.ts
Show resolved
Hide resolved
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.
Nice, much clearer this way 💟 Would be nice have getters for the different instance types
@@ -66,6 +68,13 @@ export class InstanceSettings { | |||
this.instanceRole = 'follower'; | |||
} | |||
|
|||
// TODO: read from process.env instead, and make it readonly | |||
instanceType?: InstanceType; |
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.
Could we also have getters for the instanceType
like we have for instanceRole
? I.e. isMain(Instance)
, isWebhook(Instance)
& isWorker(Instance)
. Would make the comparisons simpler
@@ -60,7 +60,7 @@ export abstract class AbstractServer { | |||
|
|||
readonly uniqueInstanceId: string; | |||
|
|||
constructor(instanceType: N8nInstanceType = 'main') { | |||
constructor(instanceType: Exclude<InstanceType, 'worker'>) { |
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.
Nice 👍 Much better documented now
@@ -630,5 +630,3 @@ export abstract class SecretsProvider { | |||
abstract hasSecret(name: string): boolean; | |||
abstract getSecretNames(): string[]; | |||
} | |||
|
|||
export type N8nInstanceType = 'main' | 'webhook' | 'worker'; |
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.
🎉
|
c571348
to
1c54691
Compare
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.
Looks good 🚀 I still feel having separate getters like isMainInstance
etc would make the comparisons cleaner
n8n
|
Project |
n8n
|
Branch Review |
generic.instanceType
|
Run status |
|
Run duration | 04m 43s |
Commit |
|
Committer | कारतोफ्फेलस्क्रिप्ट™ |
View all properties for this run ↗︎ |
Test results | |
---|---|
|
0
|
|
0
|
|
0
|
|
0
|
|
429
|
View all changes introduced in this branch ↗︎ |
✅ All Cypress E2E specs passed |
Agreed. We can do this in a separate PR 🙏🏽 |
* master: refactor(core): Set up worker server (#10814) fix(Google Vertex Chat Model Node): Clean service account private key (#10770) test(core): Mock filesystem in tests (#10823) test(core): Fix license mock in worker test (#10824) ci: Ignore certain paths for e2e tests for PRs (no-changelog) (#10533) fix(editor): Render image binary-data using img tags (#10829) refactor(core): Move `instanceType` to `InstanceSettings` (no-changelog) (#10640) ci(benchmark): Always perform az login before teardown (#10827) fix: Prevent copying workflow when copying outside of canvas (#10813) fix: Fix telemetry causing console error (#10828) refactor: Remove unused disable directives from `nodes-base` (#10825) refactor(core): Remove unused disable directives from backend packages (#10826) chore: Upgrade to TypeScript 5.6 (#10822) fix(editor): Prevent clipboard XSS injection (#10805) refactor(core): Simplify createDeferredPromise, and add tests (no-changelog) (#10811) fix(HTTP Request Tool Node): Fix subsequent tool calls reusung the same options (#10808) fix(editor): Make expression edit modal read-only in executions view (#10806) refactor(core): Move push message types to a new shared package (no-changelog) (#10742) fix(editor): Make schema view search copy more clear (#10807)
Got released with |
Summary
Context
Review / Merge checklist