Necessary for backup service
Depends on D4491
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Copied from D4491:
At a high level this looks good but
A. It looks like some of the CI workflows are failing, could you provide some context if that's to be expected?
B. There isn't a Test Plan so it's hard to verify that this works as expected. It says "used by later diff," could you specify which diff that is so I can follow the Test Plan for that?
Comment Actions
Yes, it's meant to fail right now, I could have rebased to be before the tunnelbroker changes, but it's hard to order this work in some way. I chose to do batch them logically "as needed by later changes".
I updated the test plan.
I eventually fix the gates again, but I had to change both backup and blob to do so. I tried to make the individual diffs as small as possible.
Comment Actions
Looks good, appreciate the rebasing and reordering to appease the CI.
| services/lib/src/server-base-reactors/CMakeLists.txt | ||
|---|---|---|
| 2 | We got a different minimum version here as well, can we try to push that up as high as we can without running into issues (with Android NDK or whatever)? | |
| 38 | Again, not sure this comment is necessary... but feel free to leave it in if it's consistent with all the other CMakeLists.txt files. | |
| services/lib/src/server-base-reactors/CMakeLists.txt | ||
|---|---|---|
| 2 | Having a higher minimum than necessary is also an anti-pattern. Unless we have something which requires it being bumped (e.g. fetchContent_MakeAvailable), then we shouldn't really be preemptively bumping a minimum required. | |