Conversation
common/distribution/dist.go
Outdated
| // Returns a copy of the distribution URI | ||
| URI() *url.URL | ||
| // Returns the distribution type | ||
| Type() DistType |
There was a problem hiding this comment.
The distribution type is encoded in two places, as distribution.Docker, and DistTypeDocker. I am not convinced we need this type information in two places.
There was a problem hiding this comment.
I was thinking to just use type assertions/switches and remove DistType and Type() from the Distribution interface. Or do you have other suggestions?
There was a problem hiding this comment.
I was hoping that we could leverage polymorphic behavior by using interfaces and avoid type assertions/switches as much as possible. I am hesitant to use DistType switches in upper architectual layers (although I am aware this is hard).
There was a problem hiding this comment.
I am hesitant to use DistType switches in upper architectual layers (although I am aware this is hard).
I'd also like to avoid this if possible. Some thoughts:
- Upper code needs a way to know the distribution type. e.g. for calling the right distribution handler. Possible solutions:
- Keep the
Type() DistTypefunc from the Distribution interface as now. - Use an helper function
DistType(d Distribution) (DistType, error)(removing theType() DistTypefunc from the Distribution interface)
- Keep the
- To avoid type assertions I can remove/unexport all the additional (not covered by the Distribution interface) methods from the distributions (
Appc.App() *discovery.Appfor Appc,ACIArchive.ArchiveURL() *url.URLfor ACIArchive,Docker.DockerString()Docker.SimpleDockerString() stringfor Docker) and implement generic helpers methods like
AppDiscovery(d Distribution) (*discovery.App,error) // Will work only for Appc Distribution
TransportURL(d Distribution) (*url.URL, error) // Will work only for ACIArchive and perhaps we can use the same function for future distributions like oci-image-layout
DockerString(d Distribution) (string,error) // Will work only for Docker Distribution
SimpleDockerString(d Distribution) (string,error) // Will work only for Docker Distribution
Since they will have a Distribution interface as input (to avoid type assertions) they have to do validation since they'll work only for a specific distribution type (so they will also return an error) (and quite probably they'll do an internal type assertion).
This will avoid the need for the user to know the additional provided methods of a distribution type but just use some helper methods.
Thoughts? Better Ideas?
There was a problem hiding this comment.
This is nice improvement 👍 One question regarding the "distribution handler": Couldn't this also be an interface type included in the returned struct from a potential Parse(...) ... method? Just fighting for more generalization ;-)
There was a problem hiding this comment.
One question regarding the "distribution handler": Couldn't this also be an interface type included in the returned struct from a potential Parse(...) ... method? Just fighting for more generalization ;-)
The distribution handlers, like the user friendly string and the use of distribution URIs as references in #3071 are on top of this. Every user could implement their own logic. Given this, I'd like that this could be shared be more projects since it could probably be of help also for k8s (@euank @yifan-gu) for handling multiple diatribution types (sometimes called, IMHO not correctly, image types).
sgotti
changed the title
sgotti
force-pushed
the
distribution_uri_impl
branch
from
369af1f to
8c5a3f2
Compare
|
Updated the PR.
@s-urbaniak I tried to simplify as much as possible. An extreme simplification will be to just remove the |
|
@sgotti thanks, this looks much simpler than the initial version. Although it does not include the abstractions I was hoping for, it does LGTM, thanks! |
| } | ||
| fds2, err := FullDockerString(d2.ds) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
I think this panic (and the one above) shouldn't be here, and just return false instead.
sgotti
force-pushed
the
distribution_uri_impl
branch
2 times, most recently
from
6a2392f to
a90b25e
Compare
This patch implements the distribution point concept with the three initial kind of distributions already used by rkt (appc, aci-archive, docker). It just makes the smallest required changes to the fetchers logic to make them work and pass the functional tests. This is the base for future fetchers (a future patch will refactor the fetcher logic to better fit the distribution concept) and store refactors.
sgotti
force-pushed
the
distribution_uri_impl
branch
from
a90b25e to
2d9de5f
Compare
|
Closing in favor of #3369 |
3 participants
This is the implementation of the distribution concept proposed in #2953.
This patch implements the distribution point concept with the three initial
kind of distributions already used by rkt (appc, aci-archive, docker).
It just makes the smallest required changes to the fetchers logic to make them
work and pass the functional tests.
This is the base for future fetchers (a future patch will refactor the
fetcher logic to better fit the distribution concept) and store refactors.