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
Apply unsupported feature annotations #1028
Conversation
c2acc5e
to
3996124
Compare
hco-e2e-upgrade-aws lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-azure, ci/prow/hco-e2e-upgrade-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-image-index-aws lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-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. |
/test hco-e2e-upgrade-prev-aws |
@nunnatsa: The specified target(s) for
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. |
hco-e2e-upgrade-prev-aws lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-prev-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. |
3996124
to
2f603fe
Compare
hco-e2e-image-index-aws lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-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-image-index-aws lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-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. |
/test hco-e2e-upgrade-prev-aws |
@nunnatsa: The specified target(s) for
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. |
hco-e2e-image-index-aws lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-azure, ci/prow/hco-e2e-upgrade-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. |
/test hco-e2e-upgrade-prev-aws |
@nunnatsa: The specified target(s) for
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. |
hco-e2e-image-index-aws lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-prev-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. |
922fe60
to
455338b
Compare
hco-e2e-image-index-aws lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-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. |
/test hco-e2e-upgrade-aws |
@nunnatsa: The specified target(s) for
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. |
hco-e2e-image-index-aws lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need to consider what's the plan for applying validation to the value of the json patches.. whether this will be included in this PR, in a different one, in the same/next release, etc...
pkg/controller/operands/operand.go
Outdated
|
||
func applyPatchToSpec(hc *hcov1beta1.HyperConverged, annotationName string, spec interface{}) error { | ||
if cnaoUnsupportedAnnotation, ok := hc.Annotations[annotationName]; ok { | ||
if err := applyAnnotationPatch(&spec, cnaoUnsupportedAnnotation); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get a bit confused by the exact semantics of "pointer to an interface{}"... is this assuming that all invocations pass spec
by value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get this comment, but the alternative is to write it 3-4 times for each operand spec. So this method actually apply a json path on any object, but as the usage (now) is only for specs, I think it's ok. I can change the name to obj
.
Expect(err).To(HaveOccurred()) | ||
}) | ||
|
||
It("Ensure func should create CDI object with changes from the annotation", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the significant distinction between the "Ensure func...) test cases and these the directly invoke NewCDI()
?
Is it worth the almost-duplicated test cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are unit test. One checks the NewCDI
func and one checks the ensure func. Not sure I get the comment. We need to make sure that NewCDI
works and that the ensure
uses it corrctlly.
HCO now support 3 new annotation on the HyperConverged CR, in jsonPatch format, in order to set/update fields in the KV, CDI or CNA CRs, that are not formally supported and are not part of the HhyperConverged API. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>
Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>
455338b
to
49d1c19
Compare
Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>
49d1c19
to
d89f61a
Compare
@nunnatsa: 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-upgrade-aws lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-azure, ci/prow/hco-e2e-upgrade-prev-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. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zcahana 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 |
/override-bot |
hco-e2e-image-index-azure, hco-e2e-image-index-aws lanes succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-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 now support 3 new annotation on the HyperConverged CR, in jsonPatch format, in order to set/update fields in the KV, CDI or CNA CRs, that are not formally supported and are not part of the HhyperConverged API.
Signed-off-by: Nahshon Unna-Tsameret nunnatsa@redhat.com
Release note: