Convert restorers to plugins #213
Conversation
skriss
force-pushed
the
restore-action-plugins
branch
2 times, most recently
from
dec29de to
138e184
Compare
| warning = errors.New("unable to restore PV snapshots: Ark server is not configured with a PersistentVolumeProvider") | ||
| } | ||
|
|
||
| return obj, warning, nil |
There was a problem hiding this comment.
Is having 2 error fields on return with one for warnings a Go idiom? I think I understand that it's meant to protect against callers being overzealous with if err != nil, but it looks like there's only one case here where we raise a warning in this function
There was a problem hiding this comment.
This is because we (ark) support both warnings and errors when restoring. This allows individual restore actions to distinguish as well.
| Execute(obj runtime.Unstructured, restore *api.Restore) (res runtime.Unstructured, warning error, err error) | ||
| } | ||
|
|
||
| // ResourceSelector is a collection of included/excluded namespaces, |
There was a problem hiding this comment.
We eventually should put this in a common package so we don't have it duplicated in pkg/backup
pkg/restore/restore.go
Outdated
|
|
||
| var backupNS v1.Namespace | ||
| if err := json.Unmarshal(nsBytes, &backupNS); err != nil { | ||
| return &v1.Namespace{ |
There was a problem hiding this comment.
Should we log here? Or return an error? Presumably it means the file is corrupt?
There was a problem hiding this comment.
I think logging a warning is reasonable -- I don't really see a reason to error out though. Do you?
There was a problem hiding this comment.
It just means that you have to go digging for why your labels/annotations weren't restored properly. I think logging is a good enough place to start.
| } | ||
| } | ||
|
|
||
| return &v1.Namespace{ |
There was a problem hiding this comment.
Would it make sense to start with backupNS and mutate fields as needed? That would ensure we get everything from the backed up NS, instead of "opting in" to just labels & annotations.
There was a problem hiding this comment.
hmm. not sure. with other objects we're restoring, in the resetMetadataAndStatus func we are taking the opt-in approach. If we take the alternate approach there's a whole bunch of stuff that we have to explicitly clear out of ObjectMeta (selfLink, uid, creationTimestamp, etc.)
pkg/restore/restore.go
Outdated
| } | ||
|
|
||
| if groupResource.Group == "" && groupResource.Resource == "persistentvolumes" { | ||
| // we need to remove annotations from PVs since they potentially contain |
Signed-off-by: Steve Kriss <steve@heptio.com>
skriss
force-pushed
the
restore-action-plugins
branch
from
9cee29f to
179b95c
Compare
Signed-off-by: Steve Kriss steve@heptio.com
@ncdc I'd like to do some re-testing of this before merge since I've done a bit of refactoring since my last full round, but the code is ready for review. I should be able to address comments tomorrow night on the flight home