-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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 backend config to a separate package (no-changelog) #9325
Conversation
…ing `_FILE` suffixed env variables
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.
Submitting for now but not finished
TODOs:
- Manually test envs → pass
- Manually test
N8N_CONFIG_FILES
→failpass - Manually test
_FILE
→ pass - Look closer at new decorators
- Look closer at mailer changes
|
||
@Config | ||
export class TemplateConfig { | ||
/** Overrides default HTML template for inviting new people (use full path) */ |
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.
Not clear what full path
means exactly. Maybe an example? Same below.
|
||
@Config | ||
export class EmailConfig { | ||
/** How to send emails */ |
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.
Whether to send emails?
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.
almost all the comments here are literally copy-pasted from what we have in schema.ts
. We should definitely improve all these docs, but I don't think we need to block this PR on these docs.
It might make sense to get the docs team involved once the new package has all the config variables in it.
Co-authored-by: Iván Ovejero <ivov.src@gmail.com>
Addressed most of the comments. But overall, I don't think this PR should be fixing the existing docs for these config variables. We need a more involved overhaul of these docs, collaborating with the docs team. Regarding the mailer changes, those could be a separate PR. Please let me know if you prefer it that way 🙏🏽 |
Tested {
"database": {
"type": "postgresdb",
"postgresdb": {
"database": "n8n",
"host": "localhost",
"port": "5432",
"schema": "public",
"user": "postgres",
"password": "some-password"
}
}
} And seeing this:
|
The plan was to add support for |
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.
🚀
const classMetadata = globalMetadata.get(ConfigClass); | ||
if (!classMetadata) { | ||
// eslint-disable-next-line n8n-local-rules/no-plain-errors | ||
throw new Error('Invalid config class: ' + ConfigClass.name); |
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.
Should we enable n8n-local-rules/no-plain-errors
for this package? In this case it makes no difference as failing here breaks the build and is not reportable, but for future proofing any other errors we may need to throw in this package. Maybe the rule should be live at the top lint config file and be disabled on a case-by-case basis.
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.
I'd like to see if we can make the type-checker somehow catch these kind of errors, to prevent stuff like this even reaching runtime.
ignoreTLS: !config.getEnv('userManagement.emails.smtp.startTLS'), | ||
}; | ||
constructor( | ||
globalConfig: GlobalConfig, |
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.
globalConfig: GlobalConfig, | |
readonly globalConfig: GlobalConfig, |
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.
will update in the next PR
globalConfig: GlobalConfig, | ||
private readonly logger: Logger, | ||
) { | ||
const smtpConfig = globalConfig.userManagement.emails.smtp; |
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.
Can a consumer of globalConfig
mutate it by accident? If so should we prevent this?
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.
right now, unfortunately yes. The plan is to switch to read-only proxies for the config eventually so that we can
- prevent writes
- block any callers from outside our packages from reading any config
Created ENG-134 for that.
|
||
constructor( | ||
private readonly userRepository: UserRepository, | ||
globalConfig: GlobalConfig, |
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.
globalConfig: GlobalConfig, | |
readonly globalConfig: GlobalConfig, |
config.getEnv('userManagement.emails.mode') === 'smtp' && | ||
config.getEnv('userManagement.emails.smtp.host') !== ''; | ||
const emailsConfig = globalConfig.userManagement.emails; | ||
this.isEmailSetUp = emailsConfig.mode === 'smtp' && emailsConfig.smtp.host !== ''; |
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.
We could consider 'getters' for the new config.
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.
That is also a part of the plan, to have more utility methods/getters on the config classes.
if (loggingOption) { | ||
const optionsString = config.getEnv('database.logging.options').replace(/\s+/g, ''); | ||
|
||
const optionsString = loggingConfig.options.replace(/\s+/g, ''); |
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.
Also in future we could have config doing its own validation.
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.
Yes. not only validation, but also transformation. so that JSON values could be auto-parsed, and we don't need to manually parse them, like we do for credentials.overwrite.data
, and a few other properties.
3 flaky tests on run #5797 ↗︎
Details:
5-ndv.cy.ts • 2 flaky tests
10-undo-redo.cy.ts • 1 flaky test
Review all test suite changes for PR #9325 ↗︎ |
✅ All Cypress E2E specs passed |
Got released with |
This PR moves the config related code into a separate
@n8n/config
package so that we can use the config on all backend packages (likecore
andnodes-base
) without creating a circular dependency withcli
.This PR also changes the config object to be
packages/cli/node_modules/@types/convict/index.d.ts
Every sub-section in the config is also now separately injectable, which we can use to create config objects specific to certain sections of the codebase, instead of using this one global config everywhere.
Ticket
ENG-126 & ENG-126
Review / Merge checklist