Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Invoke DeleteItemActions on backup deletion #2815

Merged
merged 27 commits into from Aug 21, 2020
Merged

Invoke DeleteItemActions on backup deletion #2815

merged 27 commits into from Aug 21, 2020

Conversation

nrb
Copy link
Contributor

@nrb nrb commented Aug 12, 2020

Closed #2400

@nrb nrb added P1 - Important Enhancement/User End-User Enhancement to Velero labels Aug 12, 2020
@nrb nrb added this to the v1.5 milestone Aug 12, 2020
ashish-amarnath
ashish-amarnath reviewed Aug 17, 2020
View reviewed changes
Copy link
Contributor

@ashish-amarnath ashish-amarnath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good so far.
I just made one comment.

internal/delete/delete_item_action.go Outdated Show resolved Hide resolved
pkg/controller/backup_deletion_controller.go Outdated Show resolved Hide resolved
@nrb nrb force-pushed the deleteitemaction-invocation branch from 19a5d7a to 6863bfd Compare August 18, 2020 21:32
@nrb
Copy link
Contributor Author

nrb commented Aug 18, 2020

@ashish-amarnath @carlisia This PR is still in draft because I have a few minor changes I want to make, and I'll have to remove the test implementation (I'll likely just copy it over to the example plugins).

Having the test implementation was helpful because I found some bugs where it wasn't actually getting invoked, but I think as it stands the code is pretty close to what will finally be reviewed, I'm just going to move a couple things around, like the creation of the filesystem and seeing about adding some tests.

@nrb nrb force-pushed the deleteitemaction-invocation branch 3 times, most recently from 967dc5b to ff5760e Compare August 20, 2020 01:44
@nrb nrb marked this pull request as ready for review August 20, 2020 01:45
@nrb
Copy link
Contributor Author

nrb commented Aug 20, 2020

Removed the test plugin from this codebase (will get the PR into the examples repo), and added a test suite based on the restore's RestoreItemAction checking.

@nrb
Copy link
Contributor Author

nrb commented Aug 20, 2020

Investigating the CI failure.

@nrb nrb requested a review from carlisia August 20, 2020 01:55
nrb added 17 commits August 19, 2020 22:19
@nrb
Add serving and listing support
cd1cd27 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Add clientbuilder config
604695e 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Add code path for when no DeleteItemActions found
bc8d51f 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Add test delete plugin to test unarchiving backups
e427ee0 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Always return proto.Empty for succesful Execute
182e12a 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Add resolution of plugins based on deleted items
ac18c03 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Register delete plugin for local testing
38930b7 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Move filesystem creation
151683d 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Correct typo
c793ad7 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Add per-item field logging
5fc1989 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Document future optimization
c58f64d 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Move GetItemFilePath to pkg/archive for shared use
f1ed10e 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Fix client_builder tests
2cb71e5 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Move tar write to pkg/test for shared use
23d24e6 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Add unit tests for invoking deleteitemaction plugins
ecaa1f6 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Update go modules
c4803a6 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Update imports
bc18d2b 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Remove test plugin
47eda13 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Add changelog
0f15b93 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb nrb force-pushed the deleteitemaction-invocation branch from 66ada04 to 0f15b93 Compare August 20, 2020 02:20
@nrb
Copy link
Contributor Author

nrb commented Aug 20, 2020

Still not sure why CI's failing, as make ci passes locally. I'll revisit in the morning.

@nrb
Update copyright year
c993fda 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Remove reference to gotest.tools package
46067da 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Document DeleteItemAction in custom plugins page
a7ea261 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@ashish-amarnath
Copy link
Contributor

CI is passing now 🎉

ashish-amarnath
ashish-amarnath reviewed Aug 20, 2020
View reviewed changes
Copy link
Contributor

@ashish-amarnath ashish-amarnath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few minor comments. PTAL

internal/delete/delete_item_action_handler.go Outdated Show resolved Hide resolved
internal/delete/delete_item_action_handler.go Show resolved Hide resolved
internal/delete/delete_item_action_handler.go Outdated Show resolved Hide resolved
internal/delete/delete_item_action_handler_test.go Show resolved Hide resolved
internal/delete/delete_item_action_handler_test.go Outdated Show resolved Hide resolved
pkg/restore/restore.go Show resolved Hide resolved
@nrb
Remove redundant nil check
e705293 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Update and comment test harness methods
5af0d6f 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Comment tests
e8f61ae 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb
Copy link
Contributor Author

nrb commented Aug 20, 2020

@ashish-amarnath I think I've addressed all your comments now, though not exactly. Some are going to be deferred for later clean up.

@carlisia Please take a look!

@nrb nrb self-assigned this Aug 20, 2020
ashish-amarnath
ashish-amarnath previously approved these changes Aug 20, 2020
View reviewed changes
Copy link
Contributor

@ashish-amarnath ashish-amarnath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

internal/delete/delete_item_action_handler.go Outdated Show resolved Hide resolved
@nrb
Move generic unmarshaling function to pkg/archive
638afc0 
Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
@nrb nrb force-pushed the deleteitemaction-invocation branch from 81eab6a to 638afc0 Compare August 20, 2020 19:49
ashish-amarnath
ashish-amarnath approved these changes Aug 20, 2020
View reviewed changes
Copy link
Contributor

@ashish-amarnath ashish-amarnath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

carlisia
carlisia approved these changes Aug 21, 2020
View reviewed changes
Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all lgtm! 👍

@carlisia carlisia merged commit 718a94a into vmware-tanzu:main Aug 21, 2020
@zubron zubron mentioned this pull request Sep 29, 2020
Closed
@georgettica georgettica mentioned this pull request Dec 23, 2020
Closed
georgettica pushed a commit to georgettica/velero that referenced this pull request Dec 23, 2020
@nrb @georgettica
Invoke DeleteItemActions on backup deletion (vmware-tanzu#2815)
2f97fec 
* Add serving and listing support

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
georgettica pushed a commit to georgettica/velero that referenced this pull request Jan 26, 2021
@nrb @georgettica
Invoke DeleteItemActions on backup deletion (vmware-tanzu#2815)
f17651e 
* Add serving and listing support

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
vadasambar pushed a commit to vadasambar/velero that referenced this pull request Feb 3, 2021
@nrb
Invoke DeleteItemActions on backup deletion (vmware-tanzu#2815)
7168a93 
* Add serving and listing support

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
dharmab pushed a commit to dharmab/velero that referenced this pull request May 25, 2021
@nrb @dharmab
Invoke DeleteItemActions on backup deletion (vmware-tanzu#2815)
e80cd09 
* Add serving and listing support

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
ywk253100 pushed a commit to ywk253100/velero that referenced this pull request Jun 29, 2021
@nrb @ywk253100
Invoke DeleteItemActions on backup deletion (vmware-tanzu#2815)
824e077 
* Add serving and listing support

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
gyaozhou pushed a commit to gyaozhou/velero-read that referenced this pull request May 14, 2022
@nrb @gyaozhou
Invoke DeleteItemActions on backup deletion (vmware-tanzu#2815)
5eec9dc 
* Add serving and listing support

Signed-off-by: Nolan Brubaker <brubakern@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlisia carlisia carlisia approved these changes

@ashish-amarnath ashish-amarnath Awaiting requested review from ashish-amarnath

Assignees

@nrb nrb

Labels
Enhancement/User End-User Enhancement to Velero
Projects
None yet
Milestone
v1.5.0
Development

Successfully merging this pull request may close these issues.

Request for new plugin kind to run custom delete logic while deletion of backup.
3 participants
@nrb @ashish-amarnath @carlisia