-
Notifications
You must be signed in to change notification settings - Fork 326
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
Improve error message for when config is not found #973
Improve error message for when config is not found #973
Conversation
But 2 fail tests in `config.test.js`. Because there expects the error to be thrown, but the implementation is only output to log. Probably a late fix for the tests.
I wanted to add a message for when a config file was not found, but it was difficult due to its structure, so I decided not to.
@nowsprinting can you use remove node_modules and rebuild using yarn? |
lib/config.js
Outdated
(configName || DEFAULT_CONFIG_NAME) + | ||
':\n' + | ||
joiValidationErrorsAsTable(error) + | ||
'\nNote: This message will also be output if the configuration file is not found. ' + |
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 be possible to say if (repoConfig == 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.
I tried it, and it seems that repoConfig
does not become null even if the config file does not exist.
Sorry, I mistake. plz wait. |
We could avoid passing in the default config and merge the default config in our self. |
Thanks, @jetersen |
@@ -24,7 +31,9 @@ module.exports.getConfig = async function getConfig({ context, configName }) { | |||
log({ | |||
context, | |||
message: | |||
'Config validation errors, please fix the following issues in release-drafter.yml:\n' + | |||
'Config validation errors, please fix the following issues in ' + |
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.
If the error relates to the file on the default branch, then this error message should mention that.
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 done exactly here:
Lines 15 to 21 in 0bc3bc5
if (repoConfig == null) { | |
const name = configName || DEFAULT_CONFIG_NAME | |
// noinspection ExceptionCaughtLocallyJS | |
throw new Error( | |
`Configuration file .github/${name} is not found. The configuration file must reside in your default branch.` | |
) | |
} |
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's if it's missing.
If it's found and invalid, which branch did it read?
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.
Then joiValidationErrorsAsTable(error)
will print out the errors?
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.
But will it mention the branch?
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 can't really know which "branch" as the config gets merged. So it could be in default branch or on the .github
repo you need to fix the 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.
Ah.
Could I convince you to reparse the configurations separately when you get an error? This wouldn't slow down the default path (beyond a slightly larger file), but would result in a much better user experience.
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 don't want to create a custom version of https://github.com/probot/octokit-plugin-config to satisfy slightly better user experience. User should be able to comprehend release-drafter config setup. They are the once setting it up. You can keep it as simple as you want or as complex as you want.
Again potentially you could have endless _extends
like 10 levels deep if you wanted.
Added the following messages to the error when an invalid configuration file:
I wanted to add a message for when a config file was not found, but it was difficult due to its structure, so I decided not to.fixedTwo failure tests in config.test.js, but that is unrelated to this fix.fixed