Add unpack interface to be used by client#6749
Conversation
|
Skipping CI for Draft Pull Request. |
|
Build succeeded.
|
dmcgowan
force-pushed
the
unpacker-interface
branch
from
3d5e873 to
a250275
Compare
|
Build succeeded.
|
dmcgowan
force-pushed
the
unpacker-interface
branch
from
a250275 to
9ba214c
Compare
|
Build succeeded.
|
dmcgowan
force-pushed
the
unpacker-interface
branch
from
9ba214c to
043e4a3
Compare
|
Build succeeded.
|
dmcgowan
force-pushed
the
unpacker-interface
branch
from
043e4a3 to
61dd515
Compare
|
Build succeeded.
|
dmcgowan
force-pushed
the
unpacker-interface
branch
from
61dd515 to
5d1afc4
Compare
|
Build succeeded.
|
pkg/unpack/unpacker.go
Outdated
| if u.SnapshotterName == "" { | ||
| return fmt.Errorf("snapshotter name must be provided for reference") | ||
| } | ||
| if u.Snapshotter == nil { | ||
| return fmt.Errorf("snapshotter must be provided to unpack") | ||
| } |
There was a problem hiding this comment.
How does that work if SnapshotterName and Snapshotter are different?
There was a problem hiding this comment.
Would the caller be responsible to set them in the consistent manner?
There was a problem hiding this comment.
Could rename SnapshotterName to SnapshotterKey and skip the gc reference when not provided. We probably should have been adding a Name() string to some of our interfaces or rely on checking for String() string so callers are forced to track the name/object pairing.
There was a problem hiding this comment.
Renamed SnapshotterName and removed it erroring. The key only needs to be consistent for the caller, but it doesn't need to cause an error.
cpuguy83
left a comment
cpuguy83
left a comment
There was a problem hiding this comment.
I'm not sure I see the benefit of defining an Unpacker interface here instead of where it would be used.
Even in the client the unpacker implementation is hard-coded (ie. no way to swap the implementation).
That said I think the rest of this is useful so the unpack implementation can be easily used elsewhere.
pkg/unpack/unpacker.go
Outdated
| } | ||
| var config = UnpackConfig{ | ||
| DuplicationSuppressor: kmutex.NewNoop(), | ||
| func NewUnpacker(ctx context.Context, cs content.Store, opts ...UnpackerOpt) (Unpacker, error) { |
There was a problem hiding this comment.
Can we have this return a real type instead of an interface?
The way this is, it tends to make it difficult to dig into the implementation when debugging/digging in from higher levels.
There was a problem hiding this comment.
Yeah, that makes sense here. The package isn't likely to have multiple implementation inside the package anyway. I could see the possibility of the unpack being proxied, but probably not even around that interface.
dmcgowan
force-pushed
the
unpacker-interface
branch
from
5d1afc4 to
8d84403
Compare
|
Build succeeded.
|
| } | ||
| var platformMatcher platforms.Matcher | ||
| if !uconfig.CheckPlatformSupported { | ||
| platformMatcher = platforms.All |
There was a problem hiding this comment.
the WithUnpackPlatform will apply platforms.All if the unpack.Platform.Platform is nil. It looks like that CheckPlatformSupported option is short circuit and useless.
There was a problem hiding this comment.
The logic here should be the same, but agreed it is kind of odd. I think it will be clearer though in the future having unpack platforms be explicit and separate from the fetcher's platform matcher if there is reason for them not to be the same.
hmm... reruning |
|
Build succeeded.
|
Move client unpacker to pkg/unpack Signed-off-by: Derek McGowan <[email protected]>
Require platforms to be non-empty to avoid no-op unpack Signed-off-by: Derek McGowan <[email protected]>
dmcgowan
force-pushed
the
unpacker-interface
branch
from
b80faae to
39692e7
Compare
|
Build succeeded.
|
7 participants
Move client unpacker to pkg/unpack
This new package will be used by both the client and a future service which does pull.