Skip to content

[MTSRE-666] feat: add uninstall phase#15

Merged
openshift-merge-robot merged 1 commit intoopenshift:mainfrom
Ajpantuso:apantuso/controller_refactor
Aug 31, 2022
Merged

[MTSRE-666] feat: add uninstall phase#15
openshift-merge-robot merged 1 commit intoopenshift:mainfrom
Ajpantuso:apantuso/controller_refactor

Conversation

@Ajpantuso
Copy link
Copy Markdown
Contributor

Summary

Overhauls reconciler and adds uninstall logic.

@openshift-ci openshift-ci Bot requested review from apahim and erdii August 30, 2022 14:30
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Aug 30, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Ajpantuso

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

The pull request process is described 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 openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2022
@Ajpantuso Ajpantuso force-pushed the apantuso/controller_refactor branch from a671ade to 8cfd02d Compare August 30, 2022 14:39
@Ajpantuso Ajpantuso changed the title feat: add uninstall phase [MTSRE-666] feat: add uninstall phase Aug 30, 2022
@Ajpantuso Ajpantuso force-pushed the apantuso/controller_refactor branch 7 times, most recently from 6f4fe13 to 24b7a76 Compare August 30, 2022 23:20
@apahim apahim requested review from nikhil-thomas and removed request for erdii August 31, 2022 07:43
@Ajpantuso Ajpantuso force-pushed the apantuso/controller_refactor branch from 24b7a76 to 3a2fb48 Compare August 31, 2022 13:25
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Aug 31, 2022

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

return &ReferenceAddonReconciler{
cfg: cfg,
phases: []phase.Phase{
NewPhaseUninstall(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

a comment mentioning that the order of phases matter (or is recommended) will be helpful for other developers. As the Uninstall phase is intented to remove resources and block (skip) further Reconcile phases.

}

var finalErr error

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

does checking len(csvs) == 0 make sense here.
It could be interpretted as either:

  • already uninstalled and uninstalltion is clean
  • was expecting some but none found (error)

May be we can return finalErr nil only if len(csvs) == 0. When there are csvs to be deleted we can delete them and requeue the event to ensure that CSVs are gone in the next reconcile loop.

@nikhil-thomas
Copy link
Copy Markdown
Member

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2022
@openshift-merge-robot openshift-merge-robot merged commit 15f9ba7 into openshift:main Aug 31, 2022
@Ajpantuso Ajpantuso deleted the apantuso/controller_refactor branch August 31, 2022 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants