feat(config): allow external config files in js/ts format #68
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
| userConfigFileOptions = { | ||
| directory: path.resolve(path.dirname(filepath || ''), args.directory || 'migrations'), | ||
| ...(config || {}), | ||
| }; | ||
|
|
||
| if (userConfigFileOptions.directory && !path.isAbsolute(userConfigFileOptions.directory)) { | ||
| userConfigFileOptions.directory = path.resolve(path.dirname(filepath || ''), userConfigFileOptions.directory); | ||
| } |
There was a problem hiding this comment.
This whole getConfig function got quite long and is hard to digest.
It seems this block is the same logic as above where configFileOptions is created. I think it could help to extract this code into a little helper function.
| if (configFileOptions.directory) { | ||
| delete userConfigFileOptions.directory; | ||
| } |
There was a problem hiding this comment.
It's not clear why the config directory takes precedence over the user config directory. Would be nice to have a comment that explains that.
| args || {} | ||
| ); | ||
|
|
||
| if (result.managementToken) { |
There was a problem hiding this comment.
This seems to be done to help with compatibility.
But could it not be that some user config set the accessToken?
So probably better to only overwrite it if it is not set?
| @@ -26,7 +26,7 @@ | |||
| "author": "Ben Zörb <benjamin.zoerb@jvm.de>", | |||
| "license": "MIT", | |||
| "engines": { | |||
| "node": ">=14" | |||
| "node": ">=18" | |||
There was a problem hiding this comment.
We need to increment the version with all those changes.
Is it done later when the whole thing is released?
This is a breaking change, is it not?
So we would require a new mayor version, right?
There was a problem hiding this comment.
Yes, would be minor but as we increased the required node version this needs to be major. Version will be bumped in the release process
| try { | ||
| let config = {}; | ||
| let filepath = ''; | ||
| if (args.config.endsWith('.js') || args.config.endsWith('.ts')) { |
There was a problem hiding this comment.
When we now support ts files, that must be documented in the readme
| @@ -40,7 +43,7 @@ const getConfig = async (args) => { | |||
| const { managementToken, activeSpaceId, activeEnvironmentId, host } = config || {}; | |||
| contentfulCliOptions = { | |||
| spaceId: activeSpaceId, | |||
| accessToken: managementToken, | |||
| managementToken: managementToken, | |||
There was a problem hiding this comment.
Probably also should be documented, that we now expect managementToken as the name (but still support accessToken?)
No description provided.