refactor: consolidate main logic of volume.List into volume.Volumes

[davidhsingyuchen]

volume.Ls does 3 things:

1. Sanitize the input according to the nerdctl CLI logic (e.g., "cannot use --size and --quiet together, ignoring --size").
2. The core logic: convert the human-readable filter strings into filter functions, apply them to the volumes, and return the resulting volumes.
3. Print the resulting volumes.

This PR extracts the core logic (i.e., step 2) into an exported function so that it can be reused by other projects, potentially not CLI ones, and they can have their own input validation logic and output format.

Appreciate any suggestion, thanks!

[AkihiroSuda]

It's confusing to have List() and Ls() with different functionalities

[AkihiroSuda]

I think you should just extend Volumes() to take options like VolumesOpt... that can be used for specifying the filters

[davidhsingyuchen]

Thanks for the review.

Does the following make sense to you?

// Export getVolumeFilterFuncs 

type VolumesOpt func([]native.Volume) []native.Volume

// Caller of FilterVolumesOpt can feed the output of the to-be-exported getVolumeFilterFuncs to this func.
func FilterVolumesOpt(labelFilterFuncs []func(*map[string]string) bool, nameFilterFuncs []func(string) bool, sizeFilterFuncs []func(int64) bool) VolumesOpt {
	return func(vols []native.Volume) []native.Volume {
		var res []native.Volume
		for _, vol := range vols {
			if volumeMatchesFilter(vol, labelFilterFuncs, nameFilterFuncs, sizeFilterFuncs) {
				res = append(res, vol)
			}
		}
		return res
	}
}

// Update Volumes() to go through all the options and return the final resulting volumes.

Or maybe getVolumeFilterFuncs can be invoked in FilterVolumesOpt(filters []string) so that we don't have to export getVolumeFilterFuncs, and the user of this library only need to call FilterVolumesOpt, but hasSizeFilter and removeSizeFilters will be needed unless we want to invoke getVolumeFilterFuncs twice.

[djdongjin]

This reminds me a question: usually a command does

1. preprocess step: process cli flag and construct options (which we're refactoring);
2. flag/option validation step: check either flags (previously) or options (after refactor), to resolve conflict options or do some filtering (e.g., here --quiet and --filter=size; in run we have some -i -d -t conflict).
3. main logic step: which I feel we should return the structs if it's a GET/LIST-kind func, or a success or fail indicator (bool, err, etc), or a PUT/DELETE-kind func.
4. postprocess step: after receiving results from 3, print as a table or json, etc.

I feel each step should be more isolated instead of mixed (by isolated I don't mean each should in a different func, but their logic/code should be colocated/not mixed). For example, here we're mixing option validation (--filter=size, --quiet) with main logic (getVolumeFilterFuncs); here we're mixing output with main logic.

Meanwhile, I guess 3 is the main step that we want to modularize and ideally be reused if someone want to use nerdctl as a dependency.

Take this (specific file) as an example, I think the main logic is List (return volume structs) Volumes based on given size and filters. So we should have

1. a function (e.g., ListCmd) similar to the Ls that (1). preprocess the options, (2). call a List func (below) by passing related options (filters, size), (3). postprocess/output the returned volume struct from (2). (each step may be a sub func).
2. a function (e.g., List) that has the main logic, i.e., list and filter (and return) volumes.

It's just my perspective and some (or all :D) may not make sense. Feel free to lmk if it doesn't make sense at all :)

WDYT @AkihiroSuda @Zheaoli

[djdongjin]

I think you should just extend Volumes() to take options like VolumesOpt... that can be used for specifying the filters.

I think we can just extend Volumes() to accept the options.Filter []string and handle the filter construction and the actual filtering steps, instead of doing filtering-related logic in (the current) List together with output logic. (I think we mean the same thing :))

[Zheaoli]

Mostly agree with @djdongjin

We should keep the semantics consistent with reaching the code readability

TL;DR;

1. I think if we put the List and Ls in the same file, I think it would make people confuse.(LsCmd or other names are not keeping the same style as other command)
2. we filter the volumes in List method, the only difference between namespace, dataRoot, address string, size bool and filter []string is that namespace, dataRoot, address string, size bool is the basic filter and filter []string is not. So I think we can just extend the Volumes

[davidhsingyuchen]

Thanks a lot for the suggestions. Added filters []string to the parameter list of Volumes() in 1c08e1f.

I think if we put the List and Ls in the same file, I think it would make people confuse.(LsCmd or other names are not keeping the same style as other command)

That's true. Regarding #1855, following the naming in this PR, we can probably add a new exported function named Images() in pkg/cmd/image, but if we want to extract the "main logic step" for other functions that are not "list-ish", what should the name be? For example, should we come up with a name on an ad-hoc basis? Or maybe we can use patterns like "ListCmd and List" consistently as essentially the "main logic step" of every functionality can be separated out? Or even another package could be created to hold the "main logic step". Need some guidance re. this part :)

Maybe we can continue the discussions in #1855, or maybe I can create an another issue to discuss the high-level approach as the scope is not limited to what's currently stated in #1855. Please let me know what works for you, thanks a lot!

[AkihiroSuda]

Thanks, but needs to be redesigned

https://github.com/containerd/nerdctl/pull/1837/files#r1068870842

[djdongjin]

can you change the argument order as

func lsPrintOutput(vols map[string]native.Volume, options *types.VolumeListCommandOptions, stdout io.Writer)

vols is the main target to be lsed; options decides the format, then stdout decides where to print.

[davidhsingyuchen]

Makes sense, fixed. UPDATE: checking the failed tests.

[davidhsingyuchen]

The tests were failing due to this line. Rebased on main and updated the signature of lsPrintOutput to fix them.

[djdongjin]

not related to this PR, but curious why this two flags conflict (--filter=size is for functionality/which volumes to list, while --quiet is for formatting/how to print). can we remove this restriction now? @AkihiroSuda

[AkihiroSuda]

Maybe yes, but should be another PR

[djdongjin]

I feel this comment not very descriptive to the functionality of the func. I feel we don't need to highlight "you can reuse it" (i.e., exporting it already indicates users can import and reuse it). Functionality comment is sufficient, something like

// Volumes returns volumes that match given filters. Each filter in filters has a format of .....

[davidhsingyuchen]

My bad, forgot to update this. Updated the function comment according to https://github.com/containerd/nerdctl#whale-nerdctl-volume-ls by removing the shell-related part.

[djdongjin]

LGTM, CI error looks not related, @davidhsingyuchen can you rebase to pick up new changes in main

[davidhsingyuchen]

LGTM, CI error looks not related, @davidhsingyuchen can you rebase to pick up new changes in main

Done, thanks!

[AkihiroSuda]

Thanks