Skip to content

Conversation

Honny1
Copy link
Member

@Honny1 Honny1 commented Aug 14, 2024

This PR implements a method for listing layers, images and containers. Method Store.List requires parameter type ListOptions that specifies the content of the return value. The return value is the structure ListResult that contains slices of layers, images or containers according to given options. The List method uses a locking mechanism so that the result is the current snapshot of the items listed.

Fixes:

@Honny1 Honny1 force-pushed the store-list-layers-images-containers branch 2 times, most recently from f790e26 to 241088a Compare August 15, 2024 09:21
@Honny1 Honny1 marked this pull request as ready for review August 15, 2024 09:58
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Thanks!

The approach looks good overall.

store.go Outdated
@@ -84,6 +84,20 @@ type ApplyStagedLayerOptions struct {
DiffOptions *drivers.ApplyDiffWithDifferOpts // Mandatory
}

// Options of List methode
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is at the top level of the package, there can be several things with a List method.

StoreListOptions? Or maybe name the method something unique like MultiList and then make this a MultiListOptions?

Similarly for the result type.

(Also, in both comments, a typo.)

@Honny1 Honny1 changed the title Create Store.List methode for listing layers, images, containers Create Store.List method for listing layers, images, containers Aug 16, 2024
@Honny1 Honny1 force-pushed the store-list-layers-images-containers branch 2 times, most recently from f6bcf4e to 9247029 Compare August 16, 2024 14:22
@Honny1 Honny1 requested a review from mtrmac August 16, 2024 14:27
@Honny1 Honny1 force-pushed the store-list-layers-images-containers branch from 9247029 to 613df15 Compare August 19, 2024 12:55
@Honny1 Honny1 requested a review from mtrmac August 19, 2024 12:59
Honny1 added 3 commits August 19, 2024 15:25
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
Signed-off-by: Jan Rodák <hony.com@seznam.cz>
@Honny1 Honny1 force-pushed the store-list-layers-images-containers branch from 613df15 to b175ede Compare August 19, 2024 13:26
@Honny1 Honny1 requested a review from mtrmac August 19, 2024 13:36
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

LGTM.

I suppose we should wait with merging this until we know that this is sufficient to fix the user; that’s containers/common#2125 I think.

@rhatdan
Copy link
Member

rhatdan commented Aug 20, 2024

/approve
/lgtm
Seems like decent functions to have.

Copy link
Contributor

openshift-ci bot commented Aug 20, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Honny1, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit e5de483 into containers:main Aug 20, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants