fix: bugs in Remote Configuration payload construction #151
Conversation
cataphract
requested review from
pablomartinezbernardo
and removed request for
a team
cataphract
force-pushed
the
glopes/rem-cfg-bugs
branch
from
4efa50a to
ebc6c37
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #151 +/- ##
==========================================
+ Coverage 94.45% 94.59% +0.13%
==========================================
Files 72 72
Lines 3805 3809 +4
==========================================
+ Hits 3594 3603 +9
+ Misses 211 206 -5 ☔ View full report in Codecov by Sentry. |
| struct UpdateTargetsVersion { | ||
| decltype(state_)& state; | ||
| std::uint64_t new_targets_version; | ||
|
|
||
| ~UpdateTargetsVersion() noexcept { | ||
| // if there was a global error, the old targets_version should be sent in | ||
| // the next request (at least this is what the system-tests expect) | ||
| if ((!state.error_message || state.error_message->empty()) && | ||
| new_targets_version != std::uint64_t(-1)) { | ||
| state.targets_version = new_targets_version; | ||
| } | ||
| } | ||
| } update_targets_version{state_, std::uint64_t(-1)}; |
There was a problem hiding this comment.
This seems a bit complex, especially when it might be simpler to just update the field after the entire process is successful. Am I missing something here?
There was a problem hiding this comment.
that's basically it. It's just that the function has multiple returns. But it can be done other ways of course (goto, new wrapping function, more nested ifs, etc.)
There was a problem hiding this comment.
Hey @cataphract, your solution is clever, but the RC code is already quite complex (which is entirely my fault) and not easy to grasp. Adding this defer behavior would create unnecessary additional cognitive load. I took the liberty of pushing a commit that I believe is simpler and easier to understand.
dmehala
changed the title
- Add and rework remote configuration tests. - Rework when client state are updated.
| REQUIRE(cached_target_files->size() == 3); | ||
|
|
||
| const auto ctf = cached_target_files->at(0); | ||
| CHECK(ctf.at("path").get<std::string_view>() == |
There was a problem hiding this comment.
tests are failing. You need a sort here
Description
Fix bugs in remote config found during validation.
Additional Notes
Jira ticket: [PROJ-IDENT]