feat: Nullable logic in code #1261
Conversation
Signed-off-by: Mateus Oliveira <[email protected]>
| @@ -15,7 +15,7 @@ require ( | |||
| github.com/operator-framework/operator-lib v0.9.0 | |||
| github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring v0.51.2 | |||
| github.com/sirupsen/logrus v1.9.0 | |||
| k8s.io/api v0.25.6 | |||
| k8s.io/api v0.25.6 // must update OADPResourceRequirements in api/v1alpha1/oadp_types.go when package updates | |||
There was a problem hiding this comment.
Can we create a unit test that turns resoureRequirements into unstructured and run deepEqual against current expected value? That way we know when change is required.
There was a problem hiding this comment.
nice idea!
Going to try to do that, thanks!
There was a problem hiding this comment.
did not find a way to converted to unstructured. How can I do it?
did this
func TestEqual(t *testing.T) {
created := oadpv1alpha1.OADPResourceRequirements{}
real := corev1.ResourceRequirements{}
createdConverted := corev1.ResourceRequirements(created)
realCoverted := oadpv1alpha1.OADPResourceRequirements(real)
fmt.Printf("Equal: %#v\n", reflect.DeepEqual(created, realCoverted))
fmt.Printf("Equal: %#v", reflect.DeepEqual(createdConverted, real))
}But I am not satisfied with the test, basically the difference is the nullable comment, so we should check that is the only difference between the structs is that comment (get go comments in structs, does not seem to be easy, from what I searched)
Another question: do we need these fields to allow to be nullable?
There was a problem hiding this comment.
Re: how to convert to unstructured... that's what velero does :)
Here's an example: https://github.com/vmware-tanzu/velero/blob/2caba3efb93c37edc73aac1301f68ba91a670a80/pkg/backup/item_backupper_test.go#L49
content, _ := runtime.DefaultUnstructuredConverter.ToUnstructured(tt.resource)There was a problem hiding this comment.
There was a problem hiding this comment.
There was a problem hiding this comment.
it may not help with nullables but it should help track field changes.
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
This comment was marked as outdated.
This comment was marked as outdated.
openshift-ci
bot
removed
the
lifecycle/stale
|
@mateusoliveira43: 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-sigs/prow repository. I understand the commands that are listed here. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
This comment was marked as outdated.
This comment was marked as outdated.
openshift-ci
bot
removed
the
lifecycle/stale
Remove logic for adding nullable fields from Makefile and add it to code.