Skip to content
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

Reorganize storageImageDestination to prioritize the private …WithOptions methods #1468

Merged
merged 12 commits into from Feb 15, 2022

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Feb 15, 2022

Another part of #1439 : modify storageImageDestination so that PutBlob is just a trivial wrapper over PutBlobWithOptions, and TryReusingBlob is just a trivial wrapper over TryReusingBlobWithOptions.

Later we’ll replace those wrappers with uses of universally-usable adapters.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

... so that it isn't in the middle of the PutBlob implementation.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…estination.PutBlob

For now this is just an alias, but we will want to remove the public
PutBlob implementation (in favor of using a generic compat wrapper).

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... allowing this to be replaced with a generic stub later.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... so that it isn't in the middle of TryReusingBlob.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It will be the primary internal "locked" implementation
point, we don't want to implement TryReusingBlob as a stack
of calls, each WithOneMember implementing only that OneMember.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... just like its caller tryReusingBlobAsPending

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... so that tryReusingBlobAsPending and
tryReusingBlobLocked are consecutive.  We will merge the two
(and TryReusingBlob will go away completley, eventually).

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
When options.SrcRef and options.LayerIndex is not set,
it amounts to the same thing.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It's the only caller now, and the semantics of "Locked"
vs. "AsPending" is hard to describe; just have one larger
function that does all kinds of lookups.  If it turns out
to be too large, we can split it to individual lookups,
instead of having the SrcRef case treated as uniquely special.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... to avoid copies.

Keep the interfaces in terms of struct values, to make the
value semantics explicit (and maybe to help the compiler a bit
to show that the data doesn't need to be on a heap).

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Copy link
Collaborator Author

mtrmac commented Feb 15, 2022

(Tests pass now, after a Skopeo CI fix.)

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice work

@vrothberg vrothberg merged commit 1ca5fad into containers:main Feb 15, 2022
@mtrmac mtrmac deleted the storage-private-primary branch February 15, 2022 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants