Skip to content

Move VS deletion to backup-finalizer-controller#255

Merged
shubham-pampattiwar merged 1 commit into
openshift:konveyor-devfrom
shubham-pampattiwar:defer-csi-vs-delete
Apr 7, 2023
Merged

Move VS deletion to backup-finalizer-controller#255
shubham-pampattiwar merged 1 commit into
openshift:konveyor-devfrom
shubham-pampattiwar:defer-csi-vs-delete

Conversation

@shubham-pampattiwar
Copy link
Copy Markdown
Member

@shubham-pampattiwar shubham-pampattiwar commented Apr 6, 2023 •
edited
Loading

veleroImageFqin: quay.io/spampatt/velero:defer-deletion
fixes openshift/oadp-operator#905

@shubham-pampattiwar shubham-pampattiwar self-assigned this Apr 6, 2023
sseago
sseago reviewed Apr 6, 2023
View reviewed changes
Comment thread pkg/controller/backup_controller.go Outdated
Comment thread pkg/controller/backup_finalizer_controller.go Outdated
@weshayutin weshayutin mentioned this pull request Apr 6, 2023
Closed
sseago
sseago approved these changes Apr 6, 2023
View reviewed changes
Copy link
Copy Markdown

@sseago sseago left a comment

Choose a reason for hiding this comment

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

This looks good. Acking for now, but we probably need to do the refactoring I was suggesting earlier to remove the duplication if we want to eventually submit this upstream.

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2023
kaovilai
kaovilai reviewed Apr 6, 2023
View reviewed changes
Comment thread pkg/controller/backup_finalizer_controller.go
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2023
@shubham-pampattiwar
Copy link
Copy Markdown
Member Author

shubham-pampattiwar commented Apr 7, 2023

@sseago Updated the PR with the requested changes, PTAL, Thank you !

sseago
sseago reviewed Apr 7, 2023
View reviewed changes
Comment thread pkg/controller/backup_finalizer_controller.go Outdated
// using goroutine here instead of waiting in CSI plugin, because it's not easy to make BackupItemAction
// parallel by now. After BackupItemAction parallel is implemented, this logic should be moved to CSI plugin
// as https://github.com/vmware-tanzu/velero-plugin-for-csi/pull/100
func (r *backupFinalizerReconciler) waitVolumeSnapshotReadyToUse(ctx context.Context,
Copy link
Copy Markdown

@sseago sseago Apr 7, 2023

Choose a reason for hiding this comment

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

You don't need these funcs anymore since you're calling the version in backup_controller.go

@sseago
Copy link
Copy Markdown

sseago commented Apr 7, 2023

@shubham-pampattiwar Looks like one more func can be removed from the finalizer controller (waitVolumeSnapshotReadyToUse) since you're calling the other version of it -- that and squash the commits and we should be good to go

@weshayutin
Copy link
Copy Markdown

weshayutin commented Apr 7, 2023

/retest-required

@sseago
Copy link
Copy Markdown

sseago commented Apr 7, 2023

/hold for final updates

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 7, 2023
@shubham-pampattiwar shubham-pampattiwar force-pushed the defer-csi-vs-delete branch 2 times, most recently from 5367d18 to 4747f0c Compare April 7, 2023 16:23
@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2023
@sseago
Copy link
Copy Markdown

sseago commented Apr 7, 2023

/unhold

@openshift-ci openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 7, 2023
@kaovilai
Copy link
Copy Markdown
Member

kaovilai commented Apr 7, 2023 

Verifying goimports - failed! Please run 'make update'.

https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_velero/255/pull-ci-openshift-velero-konveyor-dev-unit-test/1644375329797574656#1:build-log.txt%3A277

@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2023
@shubham-pampattiwar
Move VS deletion to backup-finalizer-controller
89cb27a 
pass missing params during controller registration

refactor code to avoid code duplication and fix tests

remove unused function

fix via make update
@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2023
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Apr 7, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kaovilai, shubham-pampattiwar, sseago

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Apr 7, 2023

@shubham-pampattiwar: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@shubham-pampattiwar shubham-pampattiwar merged commit 6c6cbf7 into openshift:konveyor-dev Apr 7, 2023
sseago pushed a commit that referenced this pull request Apr 11, 2023
@shubham-pampattiwar @sseago
Move VS deletion to backup-finalizer-controller (#255)
e7a5208 
pass missing params during controller registration

refactor code to avoid code duplication and fix tests

remove unused function

fix via make update
sseago pushed a commit that referenced this pull request Apr 11, 2023
@shubham-pampattiwar @sseago
Move VS deletion to backup-finalizer-controller (#255)
203d5bb 
pass missing params during controller registration

refactor code to avoid code duplication and fix tests

remove unused function

fix via make update
sseago pushed a commit that referenced this pull request Apr 24, 2023
@shubham-pampattiwar @sseago
Move VS deletion to backup-finalizer-controller (#255)
97e79f9 
pass missing params during controller registration

refactor code to avoid code duplication and fix tests

remove unused function

fix via make update
sseago pushed a commit that referenced this pull request Apr 24, 2023
@shubham-pampattiwar @sseago
Move VS deletion to backup-finalizer-controller (#255)
740b7ef 
pass missing params during controller registration

refactor code to avoid code duplication and fix tests

remove unused function

fix via make update
sseago pushed a commit that referenced this pull request Apr 24, 2023
@shubham-pampattiwar @sseago
Move VS deletion to backup-finalizer-controller (#255)
8e52bca 
pass missing params during controller registration

refactor code to avoid code duplication and fix tests

remove unused function

fix via make update
@kaovilai kaovilai mentioned this pull request Jul 11, 2023
Closed
3 tasks
sseago pushed a commit that referenced this pull request Aug 10, 2023
@shubham-pampattiwar @sseago
Move VS deletion to backup-finalizer-controller (#255)
620dac0 
pass missing params during controller registration

refactor code to avoid code duplication and fix tests

remove unused function

fix via make update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sseago sseago sseago approved these changes

@kaovilai kaovilai kaovilai approved these changes

Assignees

@sseago sseago

@kaovilai kaovilai

@shubham-pampattiwar shubham-pampattiwar

Labels

FeatureFreeze lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

datamover commits in 1.1 are on openshift/velero.. They were all dropped in master

4 participants

@shubham-pampattiwar @sseago @weshayutin @kaovilai