-
Notifications
You must be signed in to change notification settings - Fork 728
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: make original HTTP API compatible with config manager #2080
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2080 +/- ##
==========================================
- Coverage 76.7% 76.67% -0.04%
==========================================
Files 194 194
Lines 20003 20036 +33
==========================================
+ Hits 15344 15363 +19
- Misses 3503 3511 +8
- Partials 1156 1162 +6
Continue to review full report at Codecov.
|
/run-all-tests |
1 similar comment
/run-all-tests |
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.
Rest LGTM
case nil: | ||
default: | ||
// TODO: support more types | ||
v = updateItem |
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.
How about returning an error? I think we should be strict about unknown types in case of unexpected errors.
Signed-off-by: Ryan Leung <[email protected]>
Signed-off-by: Ryan Leung <[email protected]>
Signed-off-by: Ryan Leung <[email protected]>
Signed-off-by: Ryan Leung <[email protected]>
Signed-off-by: Ryan Leung <[email protected]>
Signed-off-by: Ryan Leung <[email protected]>
/run-all-tests |
Signed-off-by: Ryan Leung <[email protected]>
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.
LGTM
/run-all-tests |
What problem does this PR solve?
Part of #1977. It should be reviewed after #2049 is merged.
What is changed and how it works?
This PR makes the original config API support config manager.
Check List
Tests