Removes GVKRequired struct #400
Conversation
Signed-off-by: Mikalai Radchuk <[email protected]>
openshift-ci
bot
added
the
do-not-merge/work-in-progress
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #400 +/- ##
==========================================
- Coverage 81.64% 81.60% -0.04%
==========================================
Files 21 21
Lines 937 935 -2
==========================================
- Hits 765 763 -2
Misses 119 119
Partials 53 53
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| @@ -47,23 +47,13 @@ func (g GVK) String() string { | |||
| return fmt.Sprintf(`group:"%s" version:"%s" kind:"%s"`, g.Group, g.Version, g.Kind) | |||
| } | |||
|
|
|||
| type GVKRequired property.GVKRequired | |||
There was a problem hiding this comment.
Why do we have the type GVK property.GVK type alias above? Why do we need one of these two aliases but not both?
There was a problem hiding this comment.
@stevekuznetsov I beleive this is only for use to define String() method on it.
I see no reason for GVKRequired to exist as both are esentially the same struct\schema.
There was a problem hiding this comment.
Another guess: probably we re-define structs so we don't spread property.GVK across controller-runtime codebase.
There was a problem hiding this comment.
a) if we need aliases here, we need all of them
b) if there's a semantic difference between GVK and GVKRequired, we need to keep that difference here
c) if there's not a difference, let's go to "github.com/operator-framework/operator-registry/alpha/property" and remove GVKRequired.
so we don't spred property.GVK across controller-runtime codebase.
What does this mean? Why would we not want to do "spread"? What are the implications?
There was a problem hiding this comment.
Ha! I didn't even notice GVKRequired was an alias for property.GVKRequired.
if there's a semantic difference between GVK and GVKRequired, we need to keep that difference here
There is semantic difference between properties (they have different type), but not between structs: they both represent GVK. See:
What does this mean? Why would we not want to do "spread"? What are the implications?
Like probably there was an idea to redefine struct in operator controller to have less dependency on operator-registry? I don't know, I'm just guessing what the original author meant 🤷♂️.
There was a problem hiding this comment.
There is semantic difference between properties (they have different type), but not between structs: they both represent GVK.
If there is a semantic difference, I don't know that we can remove the use of one. Users are expected to understand the implication of each.
There was a problem hiding this comment.
We are not removing olm.gvk.required property type. User will still be using it. I'm only proposing to re-use a struct into which we unmarshal values of properties of both types olm.gvk and olm.gvk.required. GVKRequired is being converted into GVK anyway.
But I agree with you - this should be done on operator-registry side first. When creating this PR I didn't notice that that operator-registry also has two different structs for it.
There was a problem hiding this comment.
Ahh, I see. It looks like the only purpose of separate GVKRequired is to enable operator-registry to get its type using reflection and map it to a property type.
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
This reverts commit 5cbc9a0.
This reverts commit 5cbc9a0. Signed-off-by: Mikalai Radchuk <[email protected]>
This reverts commit 5cbc9a0. Signed-off-by: Mikalai Radchuk <[email protected]>
Description
It doesn't look like we need this duplication.
Reviewer Checklist