Prevent zed backup create from swallowing network errors
#568
Conversation
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
This was the substantive change. In #543 I refactored things to use takeBackup, which meant that the returns that moved into takeBackup no longer interrupted execution. I missed returning err here because I assumed that it was at the bottom; ineffassign didn't yell because of the defer blocks that take a pointer to err.
| cmdErr := f(cmd, args) | ||
| for _, handler := range errorHandlers { | ||
| cmdErr = handler(cmdErr) | ||
| } | ||
| return cmdErr |
There was a problem hiding this comment.
Changing this to allow us to add other handlers
internal/cmd/backup.go
Outdated
| if code != codes.PermissionDenied { | ||
| return err | ||
| } | ||
| return fmt.Errorf("%w: ensure that the token used for this call has all requisite permissions") |
There was a problem hiding this comment.
This should help guide users to setting the appropriate permissions
internal/cmd/backup_test.go
Outdated
| runErrs := make(chan error, 1) | ||
| go func() { | ||
| require.NoError(t, srv.Run(ctx)) | ||
| err := srv.Run(ctx) | ||
| runErrs <- err | ||
| }() | ||
| err := <-runErrs | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
These two refactors are testifyrequire lint fixes that I did as drive-bys. We don't have that linter enabled yet. I'd like to do that soon.
tstirrat15
force-pushed
the
fix-backup-error-propagation
branch
2 times, most recently
from
0286f54 to
2b0c0cd
Compare
| // NOTE: we return err here because there's cleanup being done | ||
| // in the `defer` blocks that will modify the `err` if cleanup | ||
| // fails | ||
| return err |
There was a problem hiding this comment.
This is also a meaningful change.
tstirrat15
force-pushed
the
fix-backup-error-propagation
branch
from
2b0c0cd to
12757c8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #568 +/- ##
==========================================
+ Coverage 39.28% 41.23% +1.95%
==========================================
Files 37 37
Lines 5448 5818 +370
==========================================
+ Hits 2140 2399 +259
- Misses 3063 3162 +99
- Partials 245 257 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tstirrat15
force-pushed
the
fix-backup-error-propagation
branch
from
12757c8 to
7837fd3
Compare
Co-authored-by: Maria Ines Parnisari <[email protected]>
There was a problem hiding this comment.
old
$ zed backup create
⠋ processing backup (0/-, 0 relationship/hr) [0s]
1:20PM INF finished backup elapsed=0s filtered=0 processed=0 throughput=0
vs new
./zed backup create
1:19PM WRN not calling a released version of SpiceDB version=v1.46.1-0.20251020215342-0ecaf138fce1+enterprise.devel.(devel)
⠋ processing backup (0/-, 0 relationship/hr) [0s]
1:19PM INF backup failed elapsed=0s filtered=0 processed=0 throughput=0
1:19PM ERR terminated with errors error="error receiving relationships: rpc error: code = PermissionDenied desc = unauthorized operation: ensure that the token used for this call has all requisite permissions"
LGTM 👍 please open an issue to refactor this code and write unit tests more easily
|
Tracked in #569 |
Description
We got a couple of reports of this. The errors in question were
PermissionDeniederrors, but we were effectively swallowing them because I missed returning theerrwhen I refactored this logic in #543.Changes
Will annotate
Testing
Review. See that tests still pass.