New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement private.ImageDestination in all transports, improve transport helpers #1589
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
0d8ca84
Beautify imagedestination.wrapped
mtrmac fdc8a5e
Introduce internal/private/ImageDestinationInternalOnly
mtrmac 0caf422
Add internal/imagedestination.impl, and use it in storageImageDestina…
mtrmac bf3a059
Add internal/imagedestination/stubs.NoPutBlobPartial
mtrmac 63a1360
Implement private.ImageDestination in dockerImageDestination
mtrmac e07ab58
Use BlobInfoCache2 instead of types.BlobInfoCache in internal/types
mtrmac 1799b98
Only create dirImageDestination when all checks are done
mtrmac 321d69a
Add a transportName parameter to docker/internal/tarfile.NewDestination
mtrmac 5e75f67
Implement private.ImageDestination in non-forwarding transports
mtrmac 5dd69be
Implement private.ImageDestination in oci/archive and openshift
mtrmac 0e068a4
Add stubs.ImplementsPutBlobPartial
mtrmac bebd5b6
Add stubs.NoSignatures and stubs.AlwaysSupportsSignatures, use them i…
mtrmac eed518d
Add internal/imagedestination/impl.Properties, use it to reduce boile…
mtrmac 52f1c33
Add imagedestination.impl.Properties.SupportedManifestMIMETypes
mtrmac c3e1251
Add imagedestination.impl.Properties.DesiredLayerCompression
mtrmac 85db725
Add Imagedestination.impl.Properties.AcceptsForeignLayerURLs
mtrmac File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
package archive | ||
|
||
import "github.com/containers/image/v5/internal/private" | ||
|
||
var _ private.ImageDestination = (*archiveImageDestination)(nil) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
package daemon | ||
|
||
import "github.com/containers/image/v5/internal/private" | ||
|
||
var _ private.ImageDestination = (*daemonImageDestination)(nil) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would very much appreciate an independent opinion. I’m really not sure whether using mix-ins like this is is more succinct and convenient, or surprising and inscrutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I found it exotic at the beginning but started to like it. I still have concerns how approachable the code is for new pairs of eyes though. I think that one central structure with boolean fields would be easier to digest. At the current state, properties are implicitly embedded into the structures.
In other words: I am completely OK to merge it as is. It's a dramatic improvement!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not quite sure what this refers to. My best guess is that this says that it’s very hard to find the
SupportsPutBlobPartial
value for a transport just by text search (quite true), and perhaps that it would be nice to have something broadly likeIs that what you mean? The immediate problem is that Go interfaces only contain methods, not fields. Secondarily, and less importantly,
SupportsPutBlobPartial
could get out of sync with the actual implementation.[1]The immediate problem could be handled by making
private.ImageSource
a structwhich is quite attractive in some respects, OTOH calling methods would be more annoying, and to an extent this is replacing compile-time interfaces with run-time dynamic function pointers. We would also have a separate
private.ImageSource
struct and some separatedockerImageSource
struct carrying the actual transport’s state, so that doesn’t help tracing the relationship between transport’s properties and methods much.With the mix-ins embedded into interface implementations, questions like “what implements $method for $transport” are compiler-visible and can be statically answered, though probably hard to follow with an IDE. With a struct pointing to separate interfaces, all of that data only exists dynamically at runtime.
[1] The primary motivation for the
No
/Implements
… stubs is to make it a compile-time failure to forget to add a method, or to fail to keep up with an interface change. Without that, it’s IIRC possible to change the type of a parameter in an interface, and implementations suddenly don’t match without any notice, so a run-time check likesource.(private.ImageDestinationWithPutBlobPartial)
can fail. With the stubs, and a single compile-time type check against aprivate.ImageDestination
, we can guarantee that one of the two stubs are present and that the type signatures are correct.Admittedly this might be overthinking it — there are only 10 transports after all, and we could just do careful type checks in unit tests, and review API additions carefully. Then we wouldn’t the
Supports…
methods, and we could just do interface type checks.WRT the non-
Supports
values, one possible alternative isand then we don’t have a
PropertyMethodsInitialize
, just a single method implemented in every transport (+compat.Impl
providing the single-value getters in the existing interface).One downside to this is that actual users of the transport would be more clumsy — everything would be
source.Properties().SomeValue
. A larger downside is that it would be harder and more expensive to implement “forwarding” transports likeoci/archive
forwarding tooci/layout
, orBlobCache
, especially if the forwarding transport only wanted to override some of the values — and the forwarding transport would have to collect all of the values although the caller only wants one of them.I’m fairly certain there is a better design to be found, and I’m very open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Also, the
…Initialize
convention kind of drives me nuts. It seems somewhat attractive to me: to move that to a run-time check (allow each of those stubs to check if it has been initialized as a private method, and run that check in a unit test of the transport):That has some run-time cost, but much worse it only works for transports where we can ~successfully create the source/destination object in a unit test. Which is awkward for remote clients like
docker-daemon
.Oh well, I suppose hating Go might count as a hobby…)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed elaboration, @mtrmac.
I am supportive of the idea - and think it's a really good one. I very much enjoy the fact that build tests can help review changes :)