-
Notifications
You must be signed in to change notification settings - Fork 38
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
node: add check for unmarshalling config to validate #3037
node: add check for unmarshalling config to validate #3037
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3037 +/- ##
==========================================
- Coverage 22.63% 22.62% -0.01%
==========================================
Files 791 791
Lines 58615 58616 +1
==========================================
- Hits 13265 13264 -1
- Misses 44452 44454 +2
Partials 898 898 ☔ View full report in Codecov by Sentry. |
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.
Far from being pretty, but looks like unavoidable at the same time.
Changes to go.mod
?
Can we also not throw a panic from SN and fail gracefully?
At the same time, if we're to do #2993, this won't be a problem, right? |
No,
I think yes. |
But that was an indirect import while here it's a direct one. I think we better handle panic here and leave the rest for #2993, it can be scheduled for the next release then. |
So, remove the check for unmarshalling? |
Yes. |
57d1485
to
5512bdb
Compare
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.
Changelog?
Signed-off-by: Andrey Butusov <[email protected]>
5512bdb
to
ff65a8c
Compare
This is a check in case there is a ",remain" tag in the configuration field, and if there is an incorrect data format, then the value of the field was ignored, since then there will be a check to see if this field is correct or not. This makes some errors clearer after unmarshalling.
Closes #3034.
Actually, I don't like this fix. I want the attribute field to be a list.