internal/: use operator-registry's PackageManifest type #1900
Conversation
| ) | ||
|
|
||
| func ValidatePackageManifest(pm *olmregistry.PackageManifest) error { | ||
| func ValidatePackageManifest(pm *registry.PackageManifest) error { |
There was a problem hiding this comment.
Are we duplicating the impl of the OLM object struct? Should not be better we use the object from OLM in order to keep the impl centralized there? If something in this struct changed then will not be required replicated the same impl for here?
There was a problem hiding this comment.
This is covered in the PR description I think:
OLM's PackageManifest type is used for display purposes, while operator-registry's is used for (un)marshalling package manifests.
There was a problem hiding this comment.
WDYT about we add comments in the code in order to make clear this diff purposes? Something that could help to avoid the same happened again( i mean the olmregistry.PackageManifest be used instead of registry.PackageManifest)
| {Name: "foo", CurrentCSVName: "bar"}, | ||
| } | ||
| pm := &olmregistry.PackageManifest{ | ||
| pm := ®istry.PackageManifest{ |
There was a problem hiding this comment.
It shows not be the same object impl here:
There was a problem hiding this comment.
WDYT about changing the name in SDK to reflect more why it is required here and make clear that both are not the same struct/object?
There was a problem hiding this comment.
IMO, our PackageManifest struct name is the best name to use since it follows the same pattern as structs for other scaffolded files. It's package path makes it clear enough (to me at least) what its intended purpose is.
There was a problem hiding this comment.
Hi @joelanford,
I understand why would you prefer keeping the same name. Just to clarifies, I was just wondering if we could make it more clear in order to avoid the same mistake that this Pr has the goal of a fix happened again.
Shows that it is very susceptible to face the same mistake, I mean another dev at any moment use one struct when would like to use another one since the SDK project is importing OLM at the end the dev will face 2 objects with the same name but with different purposes which shows be reducing the manutence ability.
There was a problem hiding this comment.
I see your point, but in this particular case, I don't think it's a concern because:
- its in a
scaffolddirectory tree making its purpose and use fairly clear - its internal, so it can only be imported in other SDK packages
- its fields are different than the OLM and operator-registry versions of these structs, another indication that it has a different purpose.
There was a problem hiding this comment.
It is ok. Tks for share your inputs as well.
|
/test e2e-aws-helm |
Description of the change: use
operator-registry'sPackageManifesttype over OLM's.Motivation for the change: OLM's
PackageManifesttype is used for display purposes, whileoperator-registry's is used for (un)marshalling package manifests.