Dynamically configure CSV disable-operand-delete annotation #2490
Conversation
kubevirt-bot
added
release-note
machadovilaca
force-pushed
the
dynamic-configure-csv-disable-operand-delete-annotation
branch
from
939fadc
to
d981b3b
Compare
|
hco-e2e-kv-smoke-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure 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. |
controllers/operands/csv.go
Outdated
| hcoutil "github.com/kubevirt/hyperconverged-cluster-operator/pkg/util" | ||
| ) | ||
|
|
||
| type csvHandler genericOperand |
There was a problem hiding this comment.
I think this is an overkill. Setting a single annotation, not dealing with creating the resource or with the upgrade use-case, I think it's enough to implement the Operand interface instead of using the genericOperand with a hook. The code will be shorter and more focused.
WDYT?
There was a problem hiding this comment.
updated, much more clean
although I have some failing operandHandler unit tests that I can not understand, can you take a look?
controllers/operands/csv.go
Outdated
| } | ||
|
|
||
| patch := fmt.Sprintf( | ||
| "[{\"op\": \"replace\",\"path\": \"/metadata/annotations/%s\",\"value\": \"%t\"}]\n", |
There was a problem hiding this comment.
I think this format is less confusing:
| "[{\"op\": \"replace\",\"path\": \"/metadata/annotations/%s\",\"value\": \"%t\"}]\n", | |
| `[{"op": "replace", "path": "/metadata/annotations/%s", "value": "%t"}]`, |
controllers/operands/csv.go
Outdated
| } | ||
|
|
||
| patch := fmt.Sprintf( | ||
| "[{\"op\": \"replace\",\"path\": \"/metadata/annotations/%s\",\"value\": \"%t\"}]\n", |
controllers/operands/csv.go
Outdated
|
|
||
| patch := fmt.Sprintf( | ||
| "[{\"op\": \"replace\",\"path\": \"/metadata/annotations/%s\",\"value\": \"%t\"}]\n", | ||
| strings.ReplaceAll(components.DisableOperandDeletionAnnotation, "/", "~1"), |
There was a problem hiding this comment.
I think we can have additional constant, instead of doing this dynamically.
controllers/operands/csv.go
Outdated
| } | ||
|
|
||
| patch := fmt.Sprintf( | ||
| "[{\"op\": \"replace\",\"path\": \"/metadata/annotations/%s\",\"value\": \"%t\"}]\n", |
There was a problem hiding this comment.
/metadata/annotations/%s - we can put here the actual value instead of formatting it dynamically.
|
hco-e2e-operator-sdk-sno-aws lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-sno-azure 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. |
|
hco-e2e-operator-sdk-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-operator-sdk-azure 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. |
Signed-off-by: João Vilaça <jvilaca@redhat.com>
machadovilaca
force-pushed
the
dynamic-configure-csv-disable-operand-delete-annotation
branch
from
307e3dc
to
afdf2d4
Compare
|
hco-e2e-operator-sdk-sno-aws lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-azure, ci/prow/hco-e2e-operator-sdk-gcp, ci/prow/hco-e2e-operator-sdk-sno-azure 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. |
machadovilaca
force-pushed
the
dynamic-configure-csv-disable-operand-delete-annotation
branch
2 times, most recently
from
76b89f6
to
f59ce73
Compare
|
hco-e2e-operator-sdk-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-aws 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. |
| "certified": "false", | ||
| "categories": "OpenShift Optional", | ||
| "containerImage": params.Image, | ||
| DisableOperandDeletionAnnotation: "true", |
There was a problem hiding this comment.
"console.openshift.io/disable-operand-delete"
machadovilaca
force-pushed
the
dynamic-configure-csv-disable-operand-delete-annotation
branch
from
f59ce73
to
ffe83c3
Compare
|
hco-e2e-operator-sdk-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure, ci/prow/okd-hco-e2e-upgrade-operator-sdk-gcp 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. |
|
hco-e2e-upgrade-prev-operator-sdk-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-azure, ci/prow/hco-e2e-upgrade-prev-operator-sdk-aws 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. |
|
hco-e2e-upgrade-operator-sdk-sno-aws lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-operator-sdk-azure 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. |
controllers/operands/csv_test.go
Outdated
| Context("UninstallStrategy is missing", func() { | ||
| It("should set console.openshift.io/disable-operand-delete to true", func() { | ||
| foundResource := ensure(req, hco, ci) | ||
| Expect(foundResource.Annotations[components.DisableOperandDeletionAnnotation]).To(Equal("true")) |
There was a problem hiding this comment.
Nit: In order to get a better error log from gomega (in case of failure), consider using this instead:
| Expect(foundResource.Annotations[components.DisableOperandDeletionAnnotation]).To(Equal("true")) | |
| Expect(foundResource.Annotations).To(HaveKeyWithValue(components.DisableOperandDeletionAnnotation, "true")) |
controllers/operands/csv_test.go
Outdated
| It("should set console.openshift.io/disable-operand-delete to true", func() { | ||
| hco.Spec.UninstallStrategy = hcov1beta1.HyperConvergedUninstallStrategyBlockUninstallIfWorkloadsExist | ||
| foundResource := ensure(req, hco, ci) | ||
| Expect(foundResource.Annotations[components.DisableOperandDeletionAnnotation]).To(Equal("true")) |
controllers/operands/csv_test.go
Outdated
| It("should set console.openshift.io/disable-operand-delete to true on changing from RemoveWorkloads", func() { | ||
| hco.Spec.UninstallStrategy = hcov1beta1.HyperConvergedUninstallStrategyRemoveWorkloads | ||
| foundResource := ensure(req, hco, ci) | ||
| Expect(foundResource.Annotations[components.DisableOperandDeletionAnnotation]).To(Equal("false")) |
controllers/operands/csv_test.go
Outdated
|
|
||
| hco.Spec.UninstallStrategy = hcov1beta1.HyperConvergedUninstallStrategyBlockUninstallIfWorkloadsExist | ||
| foundResource = ensure(req, hco, ci) | ||
| Expect(foundResource.Annotations[components.DisableOperandDeletionAnnotation]).To(Equal("true")) |
controllers/operands/csv_test.go
Outdated
| It("should set console.openshift.io/disable-operand-delete to false on changing from BlockUninstallIfWorkloadsExist", func() { | ||
| hco.Spec.UninstallStrategy = hcov1beta1.HyperConvergedUninstallStrategyBlockUninstallIfWorkloadsExist | ||
| foundResource := ensure(req, hco, ci) | ||
| Expect(foundResource.Annotations[components.DisableOperandDeletionAnnotation]).To(Equal("true")) |
controllers/operands/csv_test.go
Outdated
|
|
||
| hco.Spec.UninstallStrategy = hcov1beta1.HyperConvergedUninstallStrategyRemoveWorkloads | ||
| foundResource = ensure(req, hco, ci) | ||
| Expect(foundResource.Annotations[components.DisableOperandDeletionAnnotation]).To(Equal("false")) |
Signed-off-by: João Vilaça <jvilaca@redhat.com>
Signed-off-by: João Vilaça <jvilaca@redhat.com>
machadovilaca
force-pushed
the
dynamic-configure-csv-disable-operand-delete-annotation
branch
from
ffe83c3
to
3e98e4b
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
hco-e2e-operator-sdk-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure 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. |
|
hco-e2e-upgrade-operator-sdk-sno-aws lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure, ci/prow/okd-hco-e2e-upgrade-operator-sdk-aws 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. |
|
hco-e2e-upgrade-prev-operator-sdk-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-prev-operator-sdk-aws 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. |
|
/test pull-hyperconverged-cluster-operator-e2e-k8s-1.26-centos9 |
|
@nunnatsa: The specified target(s) for
The following commands are available to trigger optional jobs:
Use 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nunnatsa 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 |
kubevirt-bot
added
the
approved
|
/retest |
|
hco-e2e-kv-smoke-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure 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. |
|
@machadovilaca: The following tests failed, say
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. |
|
hco-e2e-kv-smoke-gcp lane succeeded. |
|
@nunnatsa: Overrode contexts on behalf of nunnatsa: ci/prow/hco-e2e-kv-smoke-azure 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. |
machadovilaca
deleted the
dynamic-configure-csv-disable-operand-delete-annotation
branch
|
/cherry-pick release-1.10 |
|
@machadovilaca: new pull request created: #2493 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. |
What this PR does / why we need it:
Following the work done in #1672, this PR communicates to OLM through the
console.openshift.io/disable-operand-deleteif the cascading deletion of all the workloads should be enabled.Reviewer Checklist
Jira Ticket:
Release note: