Descriptor: do not implement Describable interface #3886
Conversation
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #3886 +/- ##
==========================================
- Coverage 56.63% 56.58% -0.06%
==========================================
Files 106 106
Lines 10674 10696 +22
==========================================
+ Hits 6045 6052 +7
- Misses 3955 3967 +12
- Partials 674 677 +3
☔ View full report in Codecov by Sentry. |
Could you please elaborate? I don't see how deleting a method improves interoperability. |
There was a problem hiding this comment.
Do the AppendReference methods need to accept both a Describable and a Descriptor? Callers holding a Describable value (only schema1.Reference?) could always convert it to a Descriptor themselves.
builder.AppendReference(describable.Descriptor())And as far as I can tell, the ManifestBuilder interface is not needed. If we were to throw away that interface, the builders could accept the concrete types directly.
|
Since |
|
Can you rebase @thaJeztah |
4b146db to
d73d2fe
Compare
6b7a5a5 to
e22d113
Compare
Defining an interface on the implementer side is generally not best practice in Go code. There is no code in the distribution module which consumes a ManifestBuilder value so there is no need to define the interface in the distribution module. Export the concrete ManifestBuilder types and modify the constructors to return concrete values. Co-authored-by: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: Cory Snider <csnider@mirantis.com>
Commit cb6f002 implemented a generic Manifest interface to represent manifests in the registry and remove references to schema specific manifests. As part of this refactor, the Describable interface was introduced, which allowed for a single ManifestBuilder interface to handle both schema1 and schema2 manifests. Implementations of Describable are generally objects which can be described, not simply descriptors, but for convenience, this interface was also implemented on Descriptor in 2ff77c0. This interface served its purpose, but no longer needed for most cases; schema2 (and OCI) descriptors do not need this method, making it only needed for `schema1.Reference`, which is now deprecated. Requiring this interface to be implemented limits interoperability between distribution's Descriptor and the OCI Descriptor types, which are identical in every other way, except for the presence of the Describable interface. This patch: - Removes the `Descriptor.Descriptor()` method (no longer implementing the `Describable` interface). - Updates ManifestBuilder interface and implementations to accept either - Updates ManifestBuilder interface and implementations to accept a `Descriptor`. After this patch, the caller is responsible for changing a describable type into a descriptor; builder.AppendReference(describable.Descriptor()) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
e22d113 to
f1c8c41
Compare
|
@corhere Squashed that last commit, and I included your commit to remove the
|
Commit cb6f002 (#1268) implemented a generic Manifest interface to represent manifests in the registry and remove references to schema specific manifests.
As part of this refactor, the Describable interface was introduced, which allowed for a single ManifestBuilder interface to handle both schema1 and schema2 manifests. Implementations of Describable are generally objects which can be described, not simply descriptors, but for convenience, this interface was also implemented on Descriptor in 2ff77c0 (#1281).
This interface served its purpose, but no longer needed for most cases; schema2 (and OCI) descriptors do not need this method, making it only needed for
schema1.Reference, which is now deprecated.Requiring this interface to be implemented limits interoperability between distribution's Descriptor and the OCI Descriptor types, which are identical in every other way, except for the presence of the Describable interface.
This patch:
Descriptor.Descriptor()method (no longer implementing theDescribableinterface).Descriptor, aDescribable, or (for schema1), aschema1.Reference.