-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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(config): log error when config file has syntax error #5057
fix(config): log error when config file has syntax error #5057
Conversation
I think it’s ok to skip giving full details and instead just log the full error and say “could not parse config file”. Also ok to skip coverage if you keep it simple. |
Made the changes accordingly :) |
What does an example error now look like? |
This is how the log message is looking like. I feel it is ugly. $ babel-node --extensions ".ts,.js" -- lib/renovate.ts
FATAL: You must configure a GitHub personal access token |
I think it’s ok to be ugly if it’s rare and should happen only once. |
c8ea161
to
5663594
Compare
lib/config/file.ts
Outdated
// istanbul ignore if | ||
if (err instanceof SyntaxError) { | ||
logger.fatal(`Could not parse config file \n ${err.stack}`); | ||
return null; |
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.
Let's quit out (abort) instead of continuing. Otherwise a single typo could result in a complete change of config for 10s/100s/1000s of repos.
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 found out
-
Even if the config is wrong, with a valid token renovate continues to operate.That doesn't the fix for the problem.
-
I added the return to prevent the continuation
Do you want me to have process.exit(1) there ?
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, process.exit
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.
Made the changes accordingly.Its the best way to solve this error
Also, the error message is more clear now.
5663594
to
7f24845
Compare
🎉 This PR is included in version 19.89.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
I have implemented this solution by matching if the error is an instance of SyntaxError. Is this how it expected to work?
I am still not able to reach 100% code coverage after many attempts. Some help is much needed.
Issue #4894