ronantakizawa/codereview-bench huggingface dataset:
Repository: moby/moby
File: daemon/libnetwork/network.go (Go)

Code before:
		log.G(ctx).Errorf("failed to get network during CreateEndpoint: %v", err)
		return nil, err
	}
	n = ep.network

	ep.processOptions(options...)

	for _, llIPNet := range ep.Iface().LinkLocalAddresses() {
		if !llIPNet.IP.IsLinkLocalUnicast() {
			return nil, types.InvalidParameterErrorf("invalid link local IP address: %v", llIPNet.IP)
		}
	}

	if opt, ok := ep.generic[netlabel.MacAddress]; ok {
		if mac, ok := opt.(net.HardwareAddr); ok {
			ep.iface.mac = mac
		}
	}

	ipam, capability, err := n.getController().getIPAMDriver(n.ipamType)
	if err != nil {
		return nil, err
	}

	ep.ipamOptions = make(map[string]string)
	ep.ipamOptions[netlabel.EndpointName] = name
	if capability.RequiresMACAddress {
		if ep.iface.mac == nil {
			ep.iface.mac = netutils.GenerateRandomMAC()
		}
		ep.ipamOptions[netlabel.MacAddress] = ep.iface.mac.String()
	}

	wantIPv6 := n.enableIPv6 && !ep.disableIPv6

	if err = ep.assignAddress(ipam, n.enableIPv4, wantIPv6); err != nil {
		return nil, err
	}
	defer func() {
		if err != nil {
			ep.releaseAddress()
		}
	}()

	if err = n.addEndpoint(ctx, ep); err != nil {
		return nil, err
	}
	defer func() {
		if err != nil {
			if e := ep.deleteEndpoint(false); e != nil {
				log.G(ctx).Warnf("cleaning up endpoint failed %s : %v", name, e)

Review comment (suggestion):
This approach should be fine once https://github.com/moby/moby/pull/50815 is merged :+1:

```suggestion
	ep.ipamOptions = map[string]string{netlabel.EndpointName: name}
```

Code after:
		log.G(ctx).Errorf("failed to get network during CreateEndpoint: %v", err)
		return nil, err
	}
	n = ep.network

	ep.processOptions(options...)

	for _, llIPNet := range ep.Iface().LinkLocalAddresses() {
		if !llIPNet.IP.IsLinkLocalUnicast() {
			return nil, types.InvalidParameterErrorf("invalid link local IP address: %v", llIPNet.IP)
		}
	}

	if opt, ok := ep.generic[netlabel.MacAddress]; ok {
		if mac, ok := opt.(net.HardwareAddr); ok {
			ep.iface.mac = mac
		}
	}

	ipam, capability, err := n.getController().getIPAMDriver(n.ipamType)
	if err != nil {
		return nil, err
	}

	ep.ipamOptions = map[string]string{netlabel.EndpointName: name}
	if capability.RequiresMACAddress {
		if ep.iface.mac == nil {
			ep.iface.mac = netutils.GenerateRandomMAC()
		}
		ep.ipamOptions[netlabel.MacAddress] = ep.iface.mac.String()
	}

	wantIPv6 := n.enableIPv6 && !ep.disableIPv6

	if err = ep.assignAddress(ipam, n.enableIPv4, wantIPv6); err != nil {
		return nil, err
	}
	defer func() {
		if err != nil {
			ep.releaseAddress()
		}
	}()

	if err = n.addEndpoint(ctx, ep); err != nil {
		return nil, err
	}
	defer func() {
		if err != nil {
			if e := ep.deleteEndpoint(false); e != nil {
				log.G(ctx).Warnf("cleaning up endpoint failed %s : %v", name, e)
			}
Repository: moby/moby
File: daemon/libnetwork/types/types.go (Go)

Code before:
// Package types contains types that are common across libnetwork project
package types

import (
	"bytes"
	"fmt"
	"net"
	"strconv"
	"strings"

	"github.com/ishidawataru/sctp"
	"github.com/moby/moby/v2/errdefs"
)

// constants for the IP address type.
const (
	IP = iota // IPv4 and IPv6
	IPv4
	IPv6
)

// EncryptionKey is the libnetwork representation of the key distributed by the lead
// manager.
type EncryptionKey struct {
	Subsystem   string
	Algorithm   int32
	Key         []byte
	LamportTime uint64
}

// QosPolicy represents a quality of service policy on an endpoint
type QosPolicy struct {
	MaxEgressBandwidth uint64
}

// TransportPort represents a local Layer 4 endpoint
type TransportPort struct {
	Proto Protocol
	Port  uint16
}

// Equal checks if this instance of TransportPort is equal to the passed one
func (t *TransportPort) Equal(o *TransportPort) bool {
	if t == o {
		return true
	}


Review comment (suggestion):
FWIW happy to change this back to our own values / `iota`. I thought it would be nice to draw the analogy, but if it's a concern that this implicates they _MUST_ be the same, I can change it back (or only mention the syscall ones as comment), e.g.;

```go
const (
	IP   IPFamily = iota // Either IPv4 or IPv6, similar to [syscall.AF_UNSPEC].
	IPv4                 // Internet Protocol version 4 (IPv4), similar to [syscall.AF_INET ].
	IPv6                 // Internet Protocol version 6 (IPv6), similar to [syscall.AF_INET6 ].
)
```

Code after:
// Package types contains types that are common across libnetwork project
package types

import (
	"bytes"
	"fmt"
	"net"
	"strconv"
	"strings"
	"syscall"

	"github.com/ishidawataru/sctp"
	"github.com/moby/moby/v2/errdefs"
)

// IPFamily specify IP address type(s).
type IPFamily int

const (
	IP   IPFamily = syscall.AF_UNSPEC // Either IPv4 or IPv6.
	IPv4 IPFamily = syscall.AF_INET   // Internet Protocol version 4 (IPv4).
	IPv6 IPFamily = syscall.AF_INET6  // Internet Protocol version 6 (IPv6).
)

// EncryptionKey is the libnetwork representation of the key distributed by the lead
// manager.
type EncryptionKey struct {
	Subsystem   string
	Algorithm   int32
	Key         []byte
	LamportTime uint64
}

// QosPolicy represents a quality of service policy on an endpoint
type QosPolicy struct {
	MaxEgressBandwidth uint64
}

// TransportPort represents a local Layer 4 endpoint
type TransportPort struct {
	Proto Protocol
	Port  uint16
}

// Equal checks if this instance of TransportPort is equal to the passed one
func (t *TransportPort) Equal(o *TransportPort) bool {
	if t == o {
Repository: moby/moby
File: contrib/download-frozen-image-v2.sh (Shell)

Code before:
# repositories[busybox]='"latest": "...", "ubuntu-14.04": "..."'

# bash v4 on Windows CI requires CRLF separator... and linux doesn't seem to care either way
newlineIFS=$'\n'
major=$(echo "${BASH_VERSION%%[^0.9]}" | cut -d. -f1)
if [ "$major" -ge 4 ]; then
	newlineIFS=$'\r\n'
fi

registryBase='https://registry-1.docker.io'
authBase='https://auth.docker.io'
authService='registry.docker.io'

# https://github.com/moby/moby/issues/33700
fetch_blob() {
	local token="$1"
	shift
	local image="$1"
	shift
	local digest="$1"
	shift
	local targetFile="$1"
	shift
	local curlArgs=("$@")

	local curlHeaders
	curlHeaders="$(
		curl -S "${curlArgs[@]}" \
			-H "Authorization: Bearer $token" \
			"$registryBase/v2/$image/blobs/$digest" \
			-o "$targetFile" \
			-D-
	)"
	curlHeaders="$(echo "$curlHeaders" | tr -d '\r')"
	if grep -qE "^HTTP/[0-9].[0-9] 3" <<< "$curlHeaders"; then
		rm -f "$targetFile"

		local blobRedirect
		blobRedirect="$(echo "$curlHeaders" | awk -F ': ' 'tolower($1) == "location" { print $2; exit }')"
		if [ -z "$blobRedirect" ]; then
			echo >&2 "error: failed fetching '$image' blob '$digest'"
			echo "$curlHeaders" | head -1 >&2
			return 1
		fi

		curl -fSL "${curlArgs[@]}" \
			"$blobRedirect" \
			-o "$targetFile"
	fi
}


Review comment (bug):
We can use `-fsSL` now then I think? (like the other ones); doing so prevents capturing garbage if there's a failure (`-f` does a "fail fast" - preventing output to be captured on failure, and `-s` runs "silent" (but prints errors if they happen due to the other ones).

Code after:
# repositories[busybox]='"latest": "...", "ubuntu-14.04": "..."'

# bash v4 on Windows CI requires CRLF separator... and linux doesn't seem to care either way
newlineIFS=$'\n'
major=$(echo "${BASH_VERSION%%[^0.9]}" | cut -d. -f1)
if [ "$major" -ge 4 ]; then
	newlineIFS=$'\r\n'
fi

registryBase='https://registry-1.docker.io'
authBase='https://auth.docker.io'
authService='registry.docker.io'

# https://github.com/moby/moby/issues/33700
fetch_blob() {
	local token="$1"
	shift
	local image="$1"
	shift
	local digest="$1"
	shift
	local targetFile="$1"
	shift
	local curlArgs=("$@")

	curl -L -S "${curlArgs[@]}" \
		-H "Authorization: Bearer $token" \
		"$registryBase/v2/$image/blobs/$digest" \
		-o "$targetFile" \
		-D-
}

# handle 'application/vnd.docker.distribution.manifest.v2+json' manifest
handle_single_manifest_v2() {
	local manifestJson="$1"
	shift

	local configDigest
	configDigest="$(echo "$manifestJson" | jq --raw-output '.config.digest')"
	local imageId="${configDigest#*:}" # strip off "sha256:"

	local configFile="$imageId.json"
	fetch_blob "$token" "$image" "$configDigest" "$dir/$configFile" -s

	local layersFs
	layersFs="$(echo "$manifestJson" | jq --raw-output --compact-output '.layers[]')"
	local IFS="$newlineIFS"
	local layers
	mapfile -t layers <<< "$layersFs"
	unset IFS
Repository: moby/moby
File: daemon/internal/libcontainerd/remote/client.go (Go)

Code before:
	io, err = c.client.newDirectIO(context.Background(), fifos)
	if err != nil {
		return nil, err
	}

	if io.Stdin != nil {
		var (
			closeErr  error
			stdinOnce sync.Once
		)
		pipe := io.Stdin
		io.Stdin = ioutils.NewWriteCloserWrapper(pipe, func() error {
			stdinOnce.Do(func() {
				closeErr = pipe.Close()

				select {
				case p, ok := <-stdinCloseSync:
					if !ok {
						return
					}
					if err := closeStdin(context.Background(), p); err != nil {
						if closeErr != nil {
							closeErr = errors.Join(closeErr, err)
						} else {
							// Avoid wrapping a single error in a multierror.
							closeErr = err
						}
					}
				default:
					// The process wasn't ready. Close its stdin asynchronously.
					go func() {
						p, ok := <-stdinCloseSync
						if !ok {
							return
						}
						if err := closeStdin(context.Background(), p); err != nil {
							c.client.logger.WithError(err).
								WithField("container", c.c8dCtr.ID()).
								Error("failed to close container stdin")
						}
					}()
				}
			})
			return closeErr
		})
	}

	rio, err := attachStdio(io)
	if err != nil {
		io.Cancel()
		io.Close()

Review comment (suggestion):
`errors.Join` drops nils, so I think this can be ...

```suggestion
						closeErr = errors.Join(closeErr, err)
```


Code after:
	io, err = c.client.newDirectIO(context.Background(), fifos)
	if err != nil {
		return nil, err
	}

	if io.Stdin != nil {
		var (
			errs      []error
			stdinOnce sync.Once
		)
		pipe := io.Stdin
		io.Stdin = ioutils.NewWriteCloserWrapper(pipe, func() error {
			stdinOnce.Do(func() {
				errs = append(errs, pipe.Close())

				select {
				case p, ok := <-stdinCloseSync:
					if !ok {
						return
					}
					errs = append(errs, closeStdin(context.Background(), p))
				default:
					// The process wasn't ready. Close its stdin asynchronously.
					go func() {
						p, ok := <-stdinCloseSync
						if !ok {
							return
						}
						if err := closeStdin(context.Background(), p); err != nil {
							c.client.logger.WithError(err).
								WithField("container", c.c8dCtr.ID()).
								Error("failed to close container stdin")
						}
					}()
				}
			})
			return errors.Join(errs...)
		})
	}

	rio, err := attachStdio(io)
	if err != nil {
		io.Cancel()
		io.Close()
	}
	return rio, err
}

func closeStdin(ctx context.Context, p containerd.Process) error {
	err := p.CloseIO(ctx, containerd.WithStdinCloser)
	if err != nil && strings.Contains(err.Error(), "transport is closing") {
Repository: moby/moby
File: daemon/libnetwork/network.go (Go)

Code before:

func (n *Network) Skip() bool {
	n.mu.Lock()
	defer n.mu.Unlock()
	return !n.persist
}

func (n *Network) New() datastore.KVObject {
	n.mu.Lock()
	defer n.mu.Unlock()

	return &Network{
		ctrlr:   n.ctrlr,
		drvOnce: &sync.Once{},
		scope:   n.scope,
	}
}

// Copy returns a deep copy of the [IpamConf]. If the receiver is nil,
// Copy returns nil.
func (c *IpamConf) Copy() *IpamConf {
	if c == nil {
		return nil
	}

	icCopy := *c
	icCopy.AuxAddresses = maps.Clone(c.AuxAddresses)

	return &icCopy
}

// Copy returns a deep copy of [IpamInfo]. If the receiver is nil,
// Copy returns nil.
func (i *IpamInfo) Copy() *IpamInfo {
	if i == nil {
		return nil
	}

	var aux map[string]*net.IPNet
	if i.IPAMData.AuxAddresses != nil {
		aux = make(map[string]*net.IPNet, len(i.IPAMData.AuxAddresses))
		for k, v := range i.AuxAddresses {
			aux[k] = types.GetIPNetCopy(v)
		}
	}

	return &IpamInfo{
		PoolID: i.PoolID,
		Meta:   maps.Clone(i.Meta),
		IPAMData: driverapi.IPAMData{
			AddressSpace: i.AddressSpace,

Review comment (bug):
I think it would be slightly better to actually start from a zero-value `IpamConf` struct.

When adding a new field to the `IpamConf`:
- it's easy to overlook this place (whereas constructing a new instance of a struct is easier to find with IDE)
- Even if it's overlooked, it's easier to debug if the field in the copy will be empty instead of silently being a shallow copy.

Code after:
}

func (n *Network) Skip() bool {
	n.mu.Lock()
	defer n.mu.Unlock()
	return !n.persist
}

func (n *Network) New() datastore.KVObject {
	n.mu.Lock()
	defer n.mu.Unlock()

	return &Network{
		ctrlr:   n.ctrlr,
		drvOnce: &sync.Once{},
		scope:   n.scope,
	}
}

// Copy returns a deep copy of the [IpamConf]. If the receiver is nil,
// Copy returns nil.
func (c *IpamConf) Copy() *IpamConf {
	if c == nil {
		return nil
	}
	return &IpamConf{
		PreferredPool: c.PreferredPool,
		SubPool:       c.SubPool,
		Gateway:       c.Gateway,
		AuxAddresses:  maps.Clone(c.AuxAddresses),
	}
}

// Copy returns a deep copy of [IpamInfo]. If the receiver is nil,
// Copy returns nil.
func (i *IpamInfo) Copy() *IpamInfo {
	if i == nil {
		return nil
	}

	var aux map[string]*net.IPNet
	if i.IPAMData.AuxAddresses != nil {
		aux = make(map[string]*net.IPNet, len(i.IPAMData.AuxAddresses))
		for k, v := range i.AuxAddresses {
			aux[k] = types.GetIPNetCopy(v)
		}
	}

	return &IpamInfo{
		PoolID: i.PoolID,
		Meta:   maps.Clone(i.Meta),
Repository: moby/moby
File: daemon/devices.go (Go)

Code before:
}

func (daemon *Daemon) handleDevice(req container.DeviceRequest, spec *specs.Spec) error {
	if req.Driver == "" {
		// If no driver is explicitly requested, we iterate over the registered
		// drivers and attempt to match on capabilities.
		for driver, dd := range deviceDrivers {
			if selected := dd.capset.Match(req.Capabilities); selected != nil {
				log.G(context.TODO()).WithFields(log.Fields{
					"driver": driver,
					"capabilities": map[string]any{
						"requested": req.Capabilities,
						"selected":  selected,
					},
				}).Debugf("Selecting device driver by capabilities")
				return dd.updateSpec(spec, &deviceInstance{req: req, selectedCaps: selected})
			}
		}
	} else if dd := deviceDrivers[req.Driver]; dd != nil {
		// If a driver is explicitly requested and registered, then we use the
		// specified driver, ignoring the capabilities.
		log.G(context.TODO()).WithFields(log.Fields{
			"driver":       req.Driver,
			"capabilities": req.Capabilities,
		}).Debugf("Selecting device driver by driver name; possibly ignoring capabilities")
		return dd.updateSpec(spec, &deviceInstance{req: req})
	}

	return incompatibleDeviceRequest{req.Driver, req.Capabilities}
}


Review comment (suggestion):
@thaJeztah I moved this over from #50099 since I think it may be easier to reason about as a standalone change following the discussion in https://github.com/moby/moby/pull/50099#discussion_r2127591740.

Code after:
}

func (daemon *Daemon) handleDevice(req container.DeviceRequest, spec *specs.Spec) error {
	if req.Driver == "" {
		// If no driver is explicitly requested, we iterate over the registered
		// drivers and attempt to match on capabilities.
		for driver, dd := range deviceDrivers {
			if selected := dd.capset.Match(req.Capabilities); selected != nil {
				log.G(context.TODO()).WithFields(log.Fields{
					"driver": driver,
					"capabilities": map[string]any{
						"requested": req.Capabilities,
						"selected":  selected,
					},
				}).Debugf("Selecting device driver by capabilities")
				return dd.updateSpec(spec, &deviceInstance{req: req, selectedCaps: selected})
			}
		}
	} else if dd := deviceDrivers[req.Driver]; dd != nil {
		selected := dd.capset.Match(req.Capabilities)
		// If a driver is explicitly requested and registered, then we use the
		// specified driver, ignoring the capabilities.
		log.G(context.TODO()).WithFields(log.Fields{
			"driver": req.Driver,
			"capabilities": map[string]any{
				"requested": req.Capabilities,
				"selected":  selected,
			},
		}).Debugf("Selecting device driver by driver name; possibly ignoring capabilities")
		return dd.updateSpec(spec, &deviceInstance{req: req, selectedCaps: selected})
	}

	return incompatibleDeviceRequest{req.Driver, req.Capabilities}
}
Repository: moby/moby
File: daemon/containerd/image_prune.go (Go)

Code before:
package containerd

import (
	"context"
	"errors"
	"sort"
	"strings"

	c8dimages "github.com/containerd/containerd/v2/core/images"
	"github.com/containerd/containerd/v2/core/leases"
	"github.com/containerd/containerd/v2/pkg/tracing"
	cerrdefs "github.com/containerd/errdefs"
	"github.com/containerd/log"
	"github.com/distribution/reference"
	"github.com/moby/moby/api/types/filters"
	"github.com/moby/moby/api/types/image"
	"github.com/moby/moby/v2/daemon/container"
	"github.com/moby/moby/v2/errdefs"
	"github.com/opencontainers/go-digest"
	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
)

var imagesAcceptedFilters = map[string]bool{
	"dangling": true,
	"label":    true,
	"label!":   true,
	"until":    true,
}

// errPruneRunning is returned when a prune request is received while
// one is in progress
var errPruneRunning = errdefs.Conflict(errors.New("a prune operation is already running"))

// ImagesPrune removes unused images
func (i *ImageService) ImagesPrune(ctx context.Context, fltrs filters.Args) (*image.PruneReport, error) {
	if !i.pruneRunning.CompareAndSwap(false, true) {
		return nil, errPruneRunning
	}
	defer i.pruneRunning.Store(false)

	err := fltrs.Validate(imagesAcceptedFilters)
	if err != nil {
		return nil, err
	}

	danglingOnly, err := fltrs.GetBoolOrDefault("dangling", true)
	if err != nil {

Review comment (suggestion):
Ugh; forgot to comment.

We should try to avoid these changes; we can decide to switch the format / grouping we use, but better to do it in a single / large PR across the board. These things can be tricky when back porting.

Code after:
package containerd

import (
	"context"
	"errors"
	"sort"
	"strings"

	c8dimages "github.com/containerd/containerd/v2/core/images"
	"github.com/containerd/containerd/v2/core/leases"
	"github.com/containerd/containerd/v2/pkg/tracing"
	cerrdefs "github.com/containerd/errdefs"
	"github.com/containerd/log"
	"github.com/distribution/reference"
	"github.com/moby/moby/api/types/events"
	"github.com/moby/moby/api/types/filters"
	"github.com/moby/moby/api/types/image"
	"github.com/opencontainers/go-digest"
	ocispec "github.com/opencontainers/image-spec/specs-go/v1"

	"github.com/moby/moby/v2/daemon/container"
	"github.com/moby/moby/v2/errdefs"
)

var imagesAcceptedFilters = map[string]bool{
	"dangling": true,
	"label":    true,
	"label!":   true,
	"until":    true,
}

// errPruneRunning is returned when a prune request is received while
// one is in progress
var errPruneRunning = errdefs.Conflict(errors.New("a prune operation is already running"))

// ImagesPrune removes unused images
func (i *ImageService) ImagesPrune(ctx context.Context, fltrs filters.Args) (*image.PruneReport, error) {
	if !i.pruneRunning.CompareAndSwap(false, true) {
		return nil, errPruneRunning
	}
	defer i.pruneRunning.Store(false)

	err := fltrs.Validate(imagesAcceptedFilters)
	if err != nil {
		return nil, err
	}
Repository: moby/moby
File: api/types/registry/authconfig.go (Go)

Code before:
package registry

import "context"

// AuthHeader is the name of the header used to send encoded registry
// authorization credentials for registry operations (push/pull).
const AuthHeader = "X-Registry-Auth"

// RequestAuthConfig is a function interface that clients can supply
// to retry operations after getting an authorization error.
//
// The function must return the [AuthHeader] value ([AuthConfig]), encoded
// in base64url format ([RFC4648, section 5]), which can be decoded by
// [DecodeAuthConfig].
//
// It must return an error if the privilege request fails.
//
// [RFC4648, section 5]: https://tools.ietf.org/html/rfc4648#section-5
type RequestAuthConfig func(context.Context) (string, error)

// AuthConfig contains authorization information for connecting to a Registry.
type AuthConfig struct {
	Username string `json:"username,omitempty"`
	Password string `json:"password,omitempty"`
	Auth     string `json:"auth,omitempty"`

	// Email is an optional value associated with the username.
	// This field is deprecated and will be removed in a later

Review comment (security):
Looks like RequestAuthConfig below is only used in the client, and it's referred to through GoDoc in some places. We can probably move it as well, or even consider if it's adding value (other than documentation, which ... could probably be put in the places where the signature is used).

I haven't checked yet if it's referenced in other places though (e.g. the docker CLI or elsewhere)

Code after:
package registry

import "context"

// AuthHeader is the name of the header used to send encoded registry
// authorization credentials for registry operations (push/pull).
const AuthHeader = "X-Registry-Auth"

// RequestAuthConfig is a function interface that clients can supply
// to retry operations after getting an authorization error.
//
// The function must return the [AuthHeader] value ([AuthConfig]), encoded
// in base64url format ([RFC4648, section 5]), which can be decoded by
// [DecodeAuthConfig].
//
// It must return an error if the privilege request fails.
//
// [RFC4648, section 5]: https://tools.ietf.org/html/rfc4648#section-5
type RequestAuthConfig func(context.Context) (string, error)

// AuthConfig contains authorization information for connecting to a Registry.
type AuthConfig struct {
	Username string `json:"username,omitempty"`
	Password string `json:"password,omitempty"`
	Auth     string `json:"auth,omitempty"`

	// Email is an optional value associated with the username.
	//
Repository: moby/moby
File: testutil/environment/protect.go (Go)

Code before:
	var plugins []string
	for _, plugin := range pluginList {
		plugins = append(plugins, plugin.Name)
	}
	return plugins
}

// ProtectVolume adds the specified volume(s) to be protected in case of clean
func (e *Execution) ProtectVolume(t testing.TB, volumes ...string) {
	t.Helper()
	for _, vol := range volumes {
		e.protectedElements.volumes[vol] = struct{}{}
	}
}

// ProtectVolumes protects existing volumes from being cleaned up at the end of
// test runs
func ProtectVolumes(ctx context.Context, t testing.TB, testEnv *Execution) {
	t.Helper()
	volumes := getExistingVolumes(ctx, t, testEnv)
	testEnv.ProtectVolume(t, volumes...)
}

func getExistingVolumes(ctx context.Context, t testing.TB, testEnv *Execution) []string {
	t.Helper()
	client := testEnv.APIClient()
	volumeList, err := client.VolumeList(ctx, volume.ListOptions{})
	assert.NilError(t, err, "failed to list volumes")

	var volumes []string
	for _, vol := range volumeList.Volumes {
		volumes = append(volumes, vol.Name)
	}
	return volumes
}


Review comment (suggestion):
I made a few passes at this, but we should probably consider doing a rename for this across the board (i.e., integration, testutil, etc), and use `apiClient` as canonical name everywhere.

Code after:
	var plugins []string
	for _, plugin := range pluginList {
		plugins = append(plugins, plugin.Name)
	}
	return plugins
}

// ProtectVolume adds the specified volume(s) to be protected in case of clean
func (e *Execution) ProtectVolume(t testing.TB, volumes ...string) {
	t.Helper()
	for _, vol := range volumes {
		e.protectedElements.volumes[vol] = struct{}{}
	}
}

// ProtectVolumes protects existing volumes from being cleaned up at the end of
// test runs
func ProtectVolumes(ctx context.Context, t testing.TB, testEnv *Execution) {
	t.Helper()
	volumes := getExistingVolumes(ctx, t, testEnv)
	testEnv.ProtectVolume(t, volumes...)
}

func getExistingVolumes(ctx context.Context, t testing.TB, testEnv *Execution) []string {
	t.Helper()
	apiClient := testEnv.APIClient()
	volumeList, err := apiClient.VolumeList(ctx, client.VolumeListOptions{})
	assert.NilError(t, err, "failed to list volumes")

	var volumes []string
	for _, vol := range volumeList.Volumes {
		volumes = append(volumes, vol.Name)
	}
	return volumes
}