Handle errors in additionalItemBackupper #512
Conversation
carlpett
force-pushed
the
additional-backup-error
branch
from
922f7be to
1859063
Compare
pkg/backup/item_backupper_test.go
Outdated
| snapshotService = &arktest.FakeSnapshotService{ | ||
| SnapshottableVolumes: test.snapshottableVolumes, | ||
| VolumeID: "vol-abc123", | ||
| if test.snapshotError != nil { |
There was a problem hiding this comment.
This is a bit of a mess - would appreciate some ideas on how to make this clearer
| @@ -89,3 +89,31 @@ func (s *FakeSnapshotService) SetVolumeID(pv runtime.Unstructured, volumeID stri | |||
| s.VolumeIDSet = volumeID | |||
| return pv, nil | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Not sure if this is the right place?
There was a problem hiding this comment.
Instead of creating a new type, please add err error to FakeSnapshotService and return s.err wherever we need to return an error.
pkg/backup/item_backupper.go
Outdated
| @@ -206,7 +206,10 @@ func (ib *defaultItemBackupper) backupItem(logger logrus.FieldLogger, obj runtim | |||
| return err | |||
| } | |||
|
|
|||
| ib.additionalItemBackupper.backupItem(log, additionalItem, gvr.GroupResource()) | |||
| err = ib.additionalItemBackupper.backupItem(log, additionalItem, gvr.GroupResource()) | |||
There was a problem hiding this comment.
When we are only checking the returned error and aren't doing anything else with it, we usually use the syntax if err := foo(); err != nil {
There was a problem hiding this comment.
Right. I'll do the same for the three if:s above at the same time, then?
There was a problem hiding this comment.
Ah, sorry. That doesn't work though, since they set other vars. Never mind, then, just fixing this one :)
| @@ -89,3 +89,31 @@ func (s *FakeSnapshotService) SetVolumeID(pv runtime.Unstructured, volumeID stri | |||
| s.VolumeIDSet = volumeID | |||
| return pv, nil | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Instead of creating a new type, please add err error to FakeSnapshotService and return s.err wherever we need to return an error.
pkg/backup/item_backupper_test.go
Outdated
| snapshotService = &arktest.FakeSnapshotService{ | ||
| SnapshottableVolumes: test.snapshottableVolumes, | ||
| VolumeID: "vol-abc123", | ||
| if test.snapshotError != nil { |
Signed-off-by: Calle Pettersson <[email protected]>
carlpett
force-pushed
the
additional-backup-error
branch
from
1859063 to
24dfef6
Compare
| @@ -242,6 +243,17 @@ func TestBackupItemNoSkips(t *testing.T) { | |||
| "vol-abc123": {SnapshotID: "snapshot-1", AvailabilityZone: "us-east-1c"}, | |||
| }, | |||
| }, | |||
| { | |||
| name: "backup fails when takePVSnapshot fails", | |||
There was a problem hiding this comment.
To test your change to item_backupper.go, we need a test case that has this mocked behavior return a non-nil error: additionalItemBackupper.On("backupItem", mock.AnythingOfType("*logrus.Entry"), test.customActionAdditionalItems[i], item.GroupResource).Return(nil).
The test case you've added here is valuable too, so we should keep it, but it doesn't cover your fix: namely, that when we call ib.additionalItemBackupper.backupItem(log, additionalItem, gvr.GroupResource()), it checks the returned error and returns it if it's non-nil.
There was a problem hiding this comment.
Something like this would cover your change: https://gist.github.com/ncdc/12c3b6b6f4db0e5c60f4ecf77e8d85c2
There was a problem hiding this comment.
Thanks! Updated with this, will ask some questions on Slack too :)
Signed-off-by: Calle Pettersson <[email protected]>
|
LGTM. @skriss ptal |
Fixes #509.
Currently the corresponding test fails, which I think is because of #511.