Skip to content
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

config: Fix confusing errors #642

Merged
merged 3 commits into from Jan 14, 2019
Merged

config: Fix confusing errors #642

merged 3 commits into from Jan 14, 2019

Conversation

hsanjuan
Copy link
Collaborator

The JSON parsing of the config could error, but we skipped error checking and
use Validate() at the end. This caused that maybe some JSON parsing errors
where logged but the final error when validating the configuration came from
somewhere different, creating very confusing error messages for the user.

This changes this, along with removing hardcoded section lists. This also
removes a Sharder component section because, AFAIK, it was a left over
from the sharding branch and in the end there is no separate sharding
component that needs a config.

License: MIT
Signed-off-by: Hector Sanjuan code@hector.link

The JSON parsing of the config could error, but we skipped error checking and
use Validate() at the end. This caused that maybe some JSON parsing errors
where logged but the final error when validating the configuration came from
somewhere different, creating very confusing error messages for the user.

This changes this, along with removing hardcoded section lists. This also
removes a Sharder component section because, AFAIK, it was a left over
from the sharding branch and in the end there is no separate sharding
component that needs a config.

License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
@hsanjuan hsanjuan self-assigned this Jan 11, 2019
@ghost ghost added the status/in-progress In progress label Jan 11, 2019
@hsanjuan hsanjuan mentioned this pull request Jan 11, 2019
@coveralls
Copy link

coveralls commented Jan 11, 2019

Coverage Status

Coverage decreased (-11.9%) to 53.655% when pulling 325e31d on fix/config-errors into d80f3ee on master.

In practice, we allowed this already, because parsing failed
but Validate() succeeded.

License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
@hsanjuan
Copy link
Collaborator Author

Hmm I need to fix a small quirk with this

They should not be interpreted as 0, since that may overwrite
defaults which are not 0. We simply need to do nothing.

License: MIT
Signed-off-by: Hector Sanjuan <code@hector.link>
Copy link
Contributor

@kishansagathiya kishansagathiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two comments/questions, but except that this looks good to me. I can approve this once those are answered.

config/config.go Show resolved Hide resolved
config/util.go Show resolved Hide resolved
@hsanjuan
Copy link
Collaborator Author

I'll merge this since I'm going to release.

@hsanjuan hsanjuan merged commit d33b274 into master Jan 14, 2019
@ghost ghost removed the status/in-progress In progress label Jan 14, 2019
@hsanjuan hsanjuan deleted the fix/config-errors branch January 14, 2019 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants