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

OADP-1833: Cleanup completed VSBs in backup finalize phase #268

Closed
wants to merge 1 commit into from
Closed

OADP-1833: Cleanup completed VSBs in backup finalize phase #268

wants to merge 1 commit into from

Conversation

shubham-pampattiwar
Copy link
Member

@shubham-pampattiwar shubham-pampattiwar commented May 10, 2023 •
edited
Loading

veleroImageFqin: 'quay.io/spampatt/velero:del-vsb-final-take-2'

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 10, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented May 10, 2023 •
edited by openshift-ci bot
Loading

@shubham-pampattiwar: This pull request references OADP-1833 which is a valid jira issue.

In response to this:

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.

@shubham-pampattiwar shubham-pampattiwar mentioned this pull request May 10, 2023
Closed
@shubham-pampattiwar
Copy link
Member Author

make update runs successfully after updating the controller-gen but make test still fails :/

@kaovilai
Copy link
Member

Then you have to update controllergen in the ci dockerfile or however that's run.

@shubham-pampattiwar shubham-pampattiwar changed the base branch from konveyor-dev to oadp-1.2 May 11, 2023 19:34
@openshift-ci-robot
Copy link

openshift-ci-robot commented May 11, 2023 •
edited by openshift-ci bot
Loading

@shubham-pampattiwar: This pull request references OADP-1833 which is a valid jira issue.

In response to this:

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.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 11, 2023
@shubham-pampattiwar shubham-pampattiwar changed the base branch from oadp-1.2 to konveyor-dev May 11, 2023 19:35
@openshift-ci-robot
Copy link

openshift-ci-robot commented May 11, 2023 •
edited by openshift-ci bot
Loading

@shubham-pampattiwar: This pull request references OADP-1833 which is a valid jira issue.

In response to this:

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.

@shubham-pampattiwar shubham-pampattiwar removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 11, 2023
@sseago
Copy link

sseago commented May 12, 2023

I have one concern here -- this involves CRD changes, so we will no longer be able to say that our Velero CRDs are identical to upstream. I don't know whether any of these differences will introduce incopatibilities or not, though.

@kaovilai
Copy link
Member

Attempting to revert controller-gen changes.

@kaovilai
Copy link
Member

This is the error in question. kubernetes-sigs/controller-tools#613

@kaovilai
Copy link
Member

Fix is in kubernetes-sigs/controller-tools@32ad710 which appears first in v0.9.0.
Also looks like I can't push to this PR.

@kaovilai
Copy link
Member

This seems to work. Need a bit more work
shubham-pampattiwar#1

See Tests in #270

@shubham-pampattiwar shubham-pampattiwar self-assigned this May 17, 2023
kaovilai
kaovilai approved these changes May 17, 2023
View reviewed changes
Copy link
Member

@kaovilai kaovilai left a comment

Choose a reason for hiding this comment

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

lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 17, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented May 17, 2023 •
edited by openshift-ci bot
Loading

@shubham-pampattiwar: This pull request references OADP-1833 which is a valid jira issue.

In response to this:

veleroImageFqin: 'quay.io/spampatt/velero:del-vsb-final-take-2'

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.

@kaovilai
Copy link
Member

@shubham-pampattiwar gotta merge this yourself whenever you're ready.. no OWNERS file = no auto merge.
Tho if you still want automerge we can remove the approved label requirement.. and rely on lgtm

@sseago
Copy link

sseago commented May 17, 2023

Lets target this at oadp-1.2 directly (since we need to pull in some poentially-conflicting changes into konveyor-dev from upstream main, and then we may not need the overrides), and we can put this on konveyor-dev separately once that's done.

Otherwise, LGTM.

@sseago
Copy link

sseago commented May 17, 2023

/hold

@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 May 17, 2023
sseago
sseago approved these changes May 17, 2023
View reviewed changes
Copy link

@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.

LGTM, but re-target at oadp-1.2 before merging.

@openshift-ci
Copy link

openshift-ci bot commented May 17, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

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

@shubham-pampattiwar
OADP-1833: Cleanup completed VSBs in backup finalize phase
abd4ed0 
fix unit test

revert to using controller-gen 0.7

update vsb failed check condition
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 17, 2023
@openshift-ci
Copy link

openshift-ci bot commented May 17, 2023

New changes are detected. LGTM label has been removed.

@shubham-pampattiwar
Copy link
Member Author

As discussed here is the PR for oadp-1.2: #271

@openshift-ci
Copy link

openshift-ci bot commented May 17, 2023

@shubham-pampattiwar: all tests passed!

Full PR test history. Your PR dashboard.

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.

@sseago
Copy link

sseago commented May 18, 2023

Closing this one, since oadp-1.2 one was merged and this will need rework before moving to konveyor-dev (post-next-rebase)

@sseago sseago closed this May 18, 2023
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

@eemcmullan eemcmullan eemcmullan approved these changes

Assignees

@sseago sseago

@kaovilai kaovilai

@shubham-pampattiwar shubham-pampattiwar

@eemcmullan eemcmullan

Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@shubham-pampattiwar @openshift-ci-robot @kaovilai @sseago @eemcmullan @openshift-merge-robot