Expose creation and update timestamp in getAllConfigs API#28
Expose creation and update timestamp in getAllConfigs API#28GurtejSohi merged 2 commits intomainfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## main #28 +/- ##
============================================
+ Coverage 89.68% 90.42% +0.74%
- Complexity 105 109 +4
============================================
Files 14 14
Lines 378 397 +19
Branches 16 18 +2
============================================
+ Hits 339 359 +20
Misses 32 32
+ Partials 7 6 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
|
|
||
| message GetAllConfigsResponse { | ||
| // list of config values along with the associated contexts | ||
| // list of config values along with the associated contexts, sorted in the ascending order of their creation time |
There was a problem hiding this comment.
I think usually the client would want the configs to be returned in the order of creation time. So, added this extra condition.
There was a problem hiding this comment.
That makes sense. If we need to add explicit sort support later, we can
| int64 creation_timestamp = 3; | ||
|
|
||
| // timestamp (in milliseconds since epoch) at which this config was last updated | ||
| int64 update_timestamp = 4; |
There was a problem hiding this comment.
Haven't added these fields to getConfig API because that API can merge the configs belonging to different contexts, each of which will have different creation and update timestamps.
There was a problem hiding this comment.
That makes sense. Eventually, we may have to expose an API that doesn't merge and does expose those (wish getConfig had been named getMergedConfig or something...), but no need until we have a use case. The same argument could actually be made for this API since the main use case as I understand it is internal - the sorted results, but I'm fine either way since it's done.
| int64 creation_timestamp = 3; | ||
|
|
||
| // timestamp (in milliseconds since epoch) at which this config was last updated | ||
| int64 update_timestamp = 4; |
There was a problem hiding this comment.
That makes sense. Eventually, we may have to expose an API that doesn't merge and does expose those (wish getConfig had been named getMergedConfig or something...), but no need until we have a use case. The same argument could actually be made for this API since the main use case as I understand it is internal - the sorted results, but I'm fine either way since it's done.
Description
Addresses #25
Testing
Unit tests and integration tests
Checklist: