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.

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

Delete quickstarts when removing StorageSystem #81

Merged
merged 2 commits into from Sep 16, 2021
Merged

Delete quickstarts when removing StorageSystem #81

merged 2 commits into from Sep 16, 2021

Conversation

vbnrh
Copy link
Member

@vbnrh vbnrh commented Sep 2, 2021 •
edited by iamniting

This commit adds quickstart cleanup when deleting StorageSystem or
uninstalling the operator

Signed-off-by: Vineet Badrinath vineetbnath@gmail.com

https://bugzilla.redhat.com/show_bug.cgi?id=2000082

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 2, 2021

Hi @vbnrh. Thanks for your PR.

I'm waiting for a red-hat-storage member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

iamniting
iamniting requested changes Sep 2, 2021
View reviewed changes
controllers/delete.go Outdated Show resolved Hide resolved
controllers/delete.go Outdated Show resolved Hide resolved
controllers/delete.go Outdated Show resolved Hide resolved
controllers/delete.go Outdated Show resolved Hide resolved
@vbnrh vbnrh force-pushed the delete-quickstart branch 2 times, most recently from 7ad9825 to ec4f5fb Compare September 2, 2021 16:09
iamniting
iamniting requested changes Sep 3, 2021
View reviewed changes
controllers/delete.go Outdated Show resolved Hide resolved
controllers/quickstarts.go Show resolved Hide resolved
controllers/quickstarts.go Outdated Show resolved Hide resolved
@vbnrh vbnrh force-pushed the delete-quickstart branch 2 times, most recently from 7784def to 361b931 Compare September 3, 2021 09:24
iamniting
iamniting requested changes Sep 3, 2021
View reviewed changes
controllers/delete.go Outdated Show resolved Hide resolved
controllers/delete.go Outdated Show resolved Hide resolved
controllers/quickstarts_test.go Show resolved Hide resolved
@vbnrh vbnrh force-pushed the delete-quickstart branch 2 times, most recently from 8b0475f to 8ef34fb Compare September 3, 2021 11:39
iamniting
iamniting requested changes Sep 6, 2021
View reviewed changes
Copy link
Member

@iamniting iamniting left a comment

Choose a reason for hiding this comment

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

Also pls take a look at failures.

controllers/delete.go Outdated Show resolved Hide resolved
controllers/delete.go Outdated Show resolved Hide resolved
controllers/quickstarts_test.go Show resolved Hide resolved
@vbnrh vbnrh requested a review from iamniting September 7, 2021 11:30
@iamniting iamniting added the backport-release-4.9 Indicate PR requires to be backported label Sep 9, 2021
iamniting
iamniting requested changes Sep 9, 2021
View reviewed changes
controllers/quickstarts_test.go Show resolved Hide resolved
controllers/delete.go Outdated Show resolved Hide resolved
controllers/quickstarts_test.go Show resolved Hide resolved
controllers/quickstarts_test.go Outdated Show resolved Hide resolved
iamniting
iamniting approved these changes Sep 9, 2021
View reviewed changes
Copy link
Member

@iamniting iamniting left a comment

Choose a reason for hiding this comment

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

looks good

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 9, 2021
@iamniting
Copy link
Member

/hold

@iamniting
Copy link
Member

Content looks good to me, But I see that you have introduced a new commit for the last changes which should not be the case. Can you move back to 3 commits only?

@iamniting
Copy link
Member

/ok-to-test

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2021
@jarrpa
Copy link
Member

jarrpa commented Sep 9, 2021

/cherrypick release-4.9

@openshift-cherrypick-robot
Copy link

@jarrpa: once the present PR merges, I will cherry-pick it on top of release-4.9 in a new PR and assign it to you.

In response to this:

/cherrypick release-4.9

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.

jarrpa
jarrpa requested changes Sep 9, 2021
View reviewed changes
Copy link
Member

@jarrpa jarrpa left a comment

Choose a reason for hiding this comment

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

There's a fundamental problem with this. In ocs-operator we only ever support one StorageCluster, which is why we do the reconciliation of most resources in that controller. But here we allow for multiple StorageSystems, so it's really weird that we're checking for the status of all other StorageSystems as part of any of it. To me this almost calls for a separate controller... but I guess if the intent is that these should never exist until at least one StorageSystem exists, then... I don't entirely like it, but I guess it'll do for now.

controllers/quickstarts.go Outdated Show resolved Hide resolved
controllers/quickstarts.go Outdated Show resolved Hide resolved
controllers/quickstarts.go Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2021
@vbnrh
controllers: Delete quickstarts when removing StorageSystem
95e2480 
This commit adds quickstart cleanup when deleting all StorageSystems.

Signed-off-by: Vineet Badrinath <vineetbnath@gmail.com>
@vbnrh
controllers: Add tests for deletion of quickstarts
69e8ad3 
This commit adds tests for deleting the quickstarts

Signed-off-by: Vineet Badrinath <vineetbnath@gmail.com>
@vbnrh vbnrh requested a review from jarrpa September 9, 2021 15:21
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 9, 2021

@vbnrh: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Rerun command
ci/prow/odf-operator-bundle-e2e-aws 69e8ad3 link /test odf-operator-bundle-e2e-aws

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.

jarrpa
jarrpa approved these changes Sep 15, 2021
View reviewed changes
Copy link
Member

@jarrpa jarrpa 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 Sep 15, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 15, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iamniting, jarrpa, vbnrh

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

The pull request process is described 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

@iamniting
Copy link
Member

/hold cancel

@openshift-merge-robot openshift-merge-robot merged commit 5e83d87 into red-hat-storage:main Sep 16, 2021
@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Sep 16, 2021
Merged
@openshift-cherrypick-robot
Copy link

@jarrpa: new pull request created: #96

In response to this:

/cherrypick release-4.9

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jarrpa jarrpa jarrpa approved these changes

@iamniting iamniting iamniting approved these changes

Assignees

@jarrpa jarrpa

@iamniting iamniting

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. backport-release-4.9 Indicate PR requires to be backported lgtm Indicates that a PR is ready to be merged. ok-to-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@vbnrh @iamniting @jarrpa @openshift-cherrypick-robot @openshift-merge-robot