ronantakizawa/codereview-bench huggingface dataset:
Repository: moby/moby
File: client/image_search_opts.go (Go)

Code before:
package client

import (
	"context"

	"github.com/moby/moby/api/types/registry"
)

// ImageSearchResult wraps results returned by ImageSearch.
type ImageSearchResult struct {
	Results []registry.SearchResult
}

// ImageSearchOptions holds parameters to search images with.
type ImageSearchOptions struct {
	RegistryAuth string

	// PrivilegeFunc is a function that clients can supply to retry operations
	// after getting an authorization error. This function returns the registry
	// authentication header value in base64 encoded format, or an error if the
	// privilege request fails.
	//
	// For details, refer to [github.com/moby/moby/api/types/registry.RequestAuthConfig].
	PrivilegeFunc func(context.Context) (string, error)
	Filters       Filters
	Limit         int
}


Review comment (suggestion):
This one is a bit awkward with `Result -> Results -> Result`, but I guess it's better to keep it consistent

Code after:
package client

import (
	"context"

	"github.com/moby/moby/api/types/registry"
)

// ImageSearchResult wraps results returned by ImageSearch.
type ImageSearchResult struct {
	Items []registry.SearchResult
}

// ImageSearchOptions holds parameters to search images with.
type ImageSearchOptions struct {
	RegistryAuth string

	// PrivilegeFunc is a function that clients can supply to retry operations
	// after getting an authorization error. This function returns the registry
	// authentication header value in base64 encoded format, or an error if the
	// privilege request fails.
	//
	// For details, refer to [github.com/moby/moby/api/types/registry.RequestAuthConfig].
	PrivilegeFunc func(context.Context) (string, error)
	Filters       Filters
	Limit         int
}
Repository: moby/moby
File: client/version.go (Go)

Code before:
package client

import (
	"context"
	"encoding/json"

	"github.com/moby/moby/api/types"
)

// ServerVersion returns information of the docker client and server host.
func (cli *Client) ServerVersion(ctx context.Context) (types.Version, error) {
	resp, err := cli.get(ctx, "/version", nil, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return types.Version{}, err
	}

	var server types.Version
	err = json.NewDecoder(resp.Body).Decode(&server)
	return server, err
}


Review comment (suggestion):
Still considering if `Version` should be in here as well, then again, it's the API version that should matter, the `Version` is just the version of the binary, so .. probably not really relevant, but there was some back-and-forth on this in a discussion I had with @austinvazquez and @vvoland on Slack;

>>> It would be much nicer if components would have actual typed objects inside like:
>> 
>> The reason for it to be loosely types was that these are purely for informational purposes, and we wanted to keep it flexible to allow arbitrary components to be added (e.g. on docker desktop, or docker enterprise edition it could add information about components only available there), without that requiring the API to be updated.
So the only mandatory / contract fields are `Name` and `Version`, everything else is “whatever is useful to show”
>
> Components like Moby and API could be the exception though :sweat_smile:





Code after:
package client

import (
	"context"
	"encoding/json"

	"github.com/moby/moby/api/types"
)

// ServerVersionOptions specifies options for the server version request.
type ServerVersionOptions struct {
	// Currently no options are supported.
}

// ServerVersionResult contains information about the Docker server host.
type ServerVersionResult struct {
	// Platform is the platform (product name) the server is running on.
	Platform PlatformInfo

	// APIVersion is the highest API version supported by the server.
	APIVersion string

	// MinAPIVersion is the minimum API version the server supports.
	MinAPIVersion string

	// Components contains version information for the components making
	// up the server. Information in this field is for informational
	// purposes, and not part of the API contract.
	Components []types.ComponentVersion
}

// PlatformInfo holds information about the platform (product name) the
// server is running on.
type PlatformInfo struct {
	// Name is the name of the platform (for example, "Docker Engine - Community",
	// or "Docker Desktop 4.49.0 (208003)")
	Name string
}

// ServerVersion returns information of the Docker server host.
func (cli *Client) ServerVersion(ctx context.Context, _ ServerVersionOptions) (ServerVersionResult, error) {
	resp, err := cli.get(ctx, "/version", nil, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return ServerVersionResult{}, err
	}
Repository: moby/moby
File: client/task_inspect.go (Go)

Code before:
	Task swarm.Task
	Raw  []byte
}

// TaskInspect returns the task information and its raw representation.
func (cli *Client) TaskInspect(ctx context.Context, taskID string) (TaskInspectResult, error) {
	taskID, err := trimID("task", taskID)
	if err != nil {
		return TaskInspectResult{}, err
	}

	resp, err := cli.get(ctx, "/tasks/"+taskID, nil, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return TaskInspectResult{}, err
	}

	body, err := io.ReadAll(resp.Body)
	if err != nil {
		return TaskInspectResult{}, err
	}

	var response swarm.Task
	rdr := bytes.NewReader(body)
	err = json.NewDecoder(rdr).Decode(&response)
	return TaskInspectResult{Task: response, Raw: body}, err
}


Review comment (refactor):
This should probably use the utility we added in https://github.com/moby/moby/commit/abf5679049d29c87d8354053da3f801cb895530f


```suggestion
	var out TaskInspectResult
	out.Raw, err = decodeWithRaw(resp, &out.Task)
	return out, err
```

Code after:
package client

import (
	"context"

	"github.com/moby/moby/api/types/swarm"
)

// TaskInspectResult contains the result of a task inspection.
type TaskInspectResult struct {
	Task swarm.Task
	Raw  []byte
}

// TaskInspect returns the task information and its raw representation.
func (cli *Client) TaskInspect(ctx context.Context, taskID string) (TaskInspectResult, error) {
	taskID, err := trimID("task", taskID)
	if err != nil {
		return TaskInspectResult{}, err
	}

	resp, err := cli.get(ctx, "/tasks/"+taskID, nil, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return TaskInspectResult{}, err
	}

	var out TaskInspectResult
	out.Raw, err = decodeWithRaw(resp, &out.Task)
	return out, err
}
Repository: moby/moby
File: client/network_inspect.go (Go)

Code before:
package client

import (
	"bytes"
	"context"
	"encoding/json"
	"io"
	"net/url"

	"github.com/moby/moby/api/types/network"
)

// NetworkInspect returns the information for a specific network configured in the docker host.
func (cli *Client) NetworkInspect(ctx context.Context, networkID string, options NetworkInspectOptions) (network.Inspect, error) {
	networkResource, _, err := cli.NetworkInspectWithRaw(ctx, networkID, options)
	return networkResource, err
}

// NetworkInspectWithRaw returns the information for a specific network configured in the docker host and its raw representation.
func (cli *Client) NetworkInspectWithRaw(ctx context.Context, networkID string, options NetworkInspectOptions) (network.Inspect, []byte, error) {
	networkID, err := trimID("network", networkID)
	if err != nil {
		return network.Inspect{}, nil, err
	}
	query := url.Values{}
	if options.Verbose {
		query.Set("verbose", "true")
	}
	if options.Scope != "" {
		query.Set("scope", options.Scope)
	}

	resp, err := cli.get(ctx, "/networks/"+networkID, query, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return network.Inspect{}, nil, err
	}

Review comment (suggestion):
This one is slightly awkward but I don't have a good solution for that; the `network.Inspect` type is already a composite that embeds `Network`; you can access `response.Network.ID` directly, but of course also through `response.Network.Network.ID`

Code after:
package client

import (
	"context"
	"net/url"

	"github.com/moby/moby/api/types/network"
)

// NetworkInspectResult contains the result of a network inspection.
type NetworkInspectResult struct {
	Network network.Inspect
	Raw     []byte
}

// NetworkInspect returns the information for a specific network configured in the docker host.
func (cli *Client) NetworkInspect(ctx context.Context, networkID string, options NetworkInspectOptions) (NetworkInspectResult, error) {
	networkID, err := trimID("network", networkID)
	if err != nil {
		return NetworkInspectResult{}, err
	}
	query := url.Values{}
	if options.Verbose {
		query.Set("verbose", "true")
	}
	if options.Scope != "" {
		query.Set("scope", options.Scope)
	}

	resp, err := cli.get(ctx, "/networks/"+networkID, query, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return NetworkInspectResult{}, err
	}

	var out NetworkInspectResult
	out.Raw, err = decodeWithRaw(resp, &out.Network)
Repository: moby/moby
File: client/network_connect.go (Go)

Code before:
package client

import (
	"context"

	"github.com/moby/moby/api/types/network"
)

// NetworkConnectOptions represents the data to be used to connect a container to the
// network.
type NetworkConnectOptions struct {
	EndpointConfig *network.EndpointSettings
}

// NetworkConnectResult represents the result of a NetworkConnect operation.
type NetworkConnectResult struct {
	// Currently empty; placeholder for future fields.
}

// NetworkConnect connects a container to an existent network in the docker host.
func (cli *Client) NetworkConnect(ctx context.Context, networkID, containerID string, options NetworkConnectOptions) (NetworkConnectResult, error) {
	networkID, err := trimID("network", networkID)
	if err != nil {
		return NetworkConnectResult{}, err
	}

	containerID, err = trimID("container", containerID)
	if err != nil {
		return NetworkConnectResult{}, err
	}

	req := network.ConnectRequest{
		Container:      containerID,
		EndpointConfig: options.EndpointConfig,
	}
	resp, err := cli.post(ctx, "/networks/"+networkID+"/connect", nil, req, nil)
	defer ensureReaderClosed(resp)
	return NetworkConnectResult{}, err
}


Review comment (suggestion):
This is a good start; I'm still contemplating if;

- `containerID` should be rolled up into the options
- more controversial; if `NetworkConnect ` should either be moved, or have an equivalent for Container ("container -> connect to network")

Both ways make sense; then again; `NetworkConnect` (for containers) could become `Container Update -> add network / remove network`).

Code after:
package client

import (
	"context"

	"github.com/moby/moby/api/types/network"
)

// NetworkConnectOptions represents the data to be used to connect a container to the
// network.
type NetworkConnectOptions struct {
	Container      string
	EndpointConfig *network.EndpointSettings
}

// NetworkConnectResult represents the result of a NetworkConnect operation.
type NetworkConnectResult struct {
	// Currently empty; placeholder for future fields.
}

// NetworkConnect connects a container to an existent network in the docker host.
func (cli *Client) NetworkConnect(ctx context.Context, networkID string, options NetworkConnectOptions) (NetworkConnectResult, error) {
	networkID, err := trimID("network", networkID)
	if err != nil {
		return NetworkConnectResult{}, err
	}

	containerID, err := trimID("container", options.Container)
	if err != nil {
		return NetworkConnectResult{}, err
	}

	nc := network.ConnectRequest{
		Container:      containerID,
		EndpointConfig: options.EndpointConfig,
	}
	resp, err := cli.post(ctx, "/networks/"+networkID+"/connect", nil, nc, nil)
	defer ensureReaderClosed(resp)
	return NetworkConnectResult{}, err
}
Repository: moby/moby
File: daemon/containerd/image_inspect.go (Go)

Code before:

	var manifests []imagetypes.ManifestSummary
	if opts.Manifests {
		manifests = multi.Manifests
	}

	repoTags, repoDigests := collectRepoTagsAndDigests(ctx, tagged)

	if requestedPlatform != nil {
		target = multi.Best.Target()
	}

	resp := &imagebackend.InspectData{
		InspectResponse: imagetypes.InspectResponse{
			ID:          target.Digest.String(),
			RepoTags:    repoTags,
			Descriptor:  &target,
			RepoDigests: repoDigests,
			Size:        size,
			Manifests:   manifests,
			Metadata: imagetypes.Metadata{
				LastTagTime: lastUpdated,
			},
		},
		// GraphDriver is omitted in API v1.52 unless using a graphdriver.
		GraphDriver: &storage.DriverData{Name: i.snapshotter},
		// Parent is deprecated with the legacy builder, but returned by the API if present.
		Parent: parent,
	}

	if multi.Best != nil {
		imgConfig := img.Config
		resp.Author = img.Author
		resp.Config = &imgConfig
		resp.Architecture = img.Architecture
		resp.Variant = img.Variant
		resp.Os = img.OS
		resp.OsVersion = img.OSVersion

		if len(img.History) > 0 {
			resp.Comment = img.History[len(img.History)-1].Comment
		}

		if img.Created != nil {
			resp.Created = img.Created.Format(time.RFC3339Nano)
		}

		resp.RootFS = imagetypes.RootFS{
			Type: img.RootFS.Type,
		}
		for _, layer := range img.RootFS.DiffIDs {

Review comment (suggestion):
I'll have a quick look on my machine, but I suspect we may not need the field here, as with snapshotters, the name of the driver is available in the response, only in a different field, so we could handle it in the router based on API version (on API < v1.52 copy the field to the GraphDriver struct);
https://github.com/moby/moby/blob/79912d4c7f40c9432a83157e6834098d60729351/daemon/inspect.go#L157-L172

Code after:

	var manifests []imagetypes.ManifestSummary
	if opts.Manifests {
		manifests = multi.Manifests
	}

	repoTags, repoDigests := collectRepoTagsAndDigests(ctx, tagged)

	if requestedPlatform != nil {
		target = multi.Best.Target()
	}

	resp := &imagebackend.InspectData{
		InspectResponse: imagetypes.InspectResponse{
			ID:          target.Digest.String(),
			RepoTags:    repoTags,
			Descriptor:  &target,
			RepoDigests: repoDigests,
			Size:        size,
			Manifests:   manifests,
			Metadata: imagetypes.Metadata{
				LastTagTime: lastUpdated,
			},
		},
		Parent: parent, // field is deprecated with the legacy builder, but returned by the API if present.

		// GraphDriver is omitted in API v1.52 unless using a graphdriver.
		GraphDriverLegacy: &storage.DriverData{Name: i.snapshotter},
	}

	if multi.Best != nil {
		imgConfig := img.Config
		resp.Author = img.Author
		resp.Config = &imgConfig
		resp.Architecture = img.Architecture
		resp.Variant = img.Variant
		resp.Os = img.OS
		resp.OsVersion = img.OSVersion

		if len(img.History) > 0 {
			resp.Comment = img.History[len(img.History)-1].Comment
		}

		if img.Created != nil {
			resp.Created = img.Created.Format(time.RFC3339Nano)
		}

		resp.RootFS = imagetypes.RootFS{
			Type: img.RootFS.Type,
		}
		for _, layer := range img.RootFS.DiffIDs {
Repository: moby/moby
File: client/volume_list.go (Go)

Code before:
package client

import (
	"context"
	"encoding/json"
	"net/url"

	"github.com/moby/moby/api/types/volume"
)

// VolumeListOptions holds parameters to list volumes.
type VolumeListOptions struct {
	Filters Filters
}

// VolumeListResult holds the result from the [Client.VolumeList] method.
type VolumeListResult struct {
	Items volume.ListResponse
}

// VolumeList returns the volumes configured in the docker host.
func (cli *Client) VolumeList(ctx context.Context, options VolumeListOptions) (VolumeListResult, error) {
	query := url.Values{}

	options.Filters.updateURLValues(query)
	resp, err := cli.get(ctx, "/volumes", query, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return VolumeListResult{}, err
	}

	var res VolumeListResult
	err = json.NewDecoder(resp.Body).Decode(&res.Items)
	return res, err
}


Review comment (suggestion):
FWIW for the client, I changed this to a slice of values instead of pointers. Happy to change to pointers if anyone can come up with good reasons why it should be a pointer 😅 

https://github.com/moby/moby/blob/8222a3f1d93d5ed7fcb9e2dd9ce40521a07859b0/api/types/volume/list_response.go#L13-L16

Code after:
package client

import (
	"context"
	"encoding/json"
	"net/url"
	"slices"

	"github.com/moby/moby/api/types/volume"
)

// VolumeListOptions holds parameters to list volumes.
type VolumeListOptions struct {
	Filters Filters
}

// VolumeListResult holds the result from the [Client.VolumeList] method.
type VolumeListResult struct {
	// List of volumes.
	Items []volume.Volume

	// Warnings that occurred when fetching the list of volumes.
	Warnings []string
}

// VolumeList returns the volumes configured in the docker host.
func (cli *Client) VolumeList(ctx context.Context, options VolumeListOptions) (VolumeListResult, error) {
	query := url.Values{}

	options.Filters.updateURLValues(query)
	resp, err := cli.get(ctx, "/volumes", query, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return VolumeListResult{}, err
	}

	var apiResp volume.ListResponse
	err = json.NewDecoder(resp.Body).Decode(&apiResp)
	if err != nil {
		return VolumeListResult{}, err
	}

	res := VolumeListResult{
		Items:    make([]volume.Volume, 0, len(apiResp.Volumes)),
		Warnings: slices.Clone(apiResp.Warnings),
Repository: moby/moby
File: client/login.go (Go)

Code before:
package client

import (
	"context"
	"encoding/json"
	"net/url"

	"github.com/moby/moby/api/types/registry"
)

// RegistryLogin authenticates the docker server with a given docker registry.
// It returns unauthorizedError when the authentication fails.
func (cli *Client) RegistryLogin(ctx context.Context, auth registry.AuthConfig) (registry.AuthenticateOKBody, error) {
	resp, err := cli.post(ctx, "/auth", url.Values{}, auth, nil)
	defer ensureReaderClosed(resp)

	if err != nil {
		return registry.AuthenticateOKBody{}, err
	}

	var response registry.AuthenticateOKBody
	err = json.NewDecoder(resp.Body).Decode(&response)
	return response, err
}


Review comment (security):
I'll open a follow-up PR to rename this type to something cleaner (`AuthenticateResponse` probably?)

Code after:
package client

import (
	"context"
	"encoding/json"
	"net/url"

	"github.com/moby/moby/api/types/registry"
)

type RegistryLoginOptions struct {
	Username      string
	Password      string
	ServerAddress string
	IdentityToken string
	RegistryToken string
}

// RegistryLoginResult holds the result of a RegistryLogin query.
type RegistryLoginResult struct {
	Auth registry.AuthenticateOKBody
}

// RegistryLogin authenticates the docker server with a given docker registry.
// It returns unauthorizedError when the authentication fails.
func (cli *Client) RegistryLogin(ctx context.Context, options RegistryLoginOptions) (RegistryLoginResult, error) {
	auth := registry.AuthConfig{
		Username:      options.Username,
		Password:      options.Password,
		ServerAddress: options.ServerAddress,
		IdentityToken: options.IdentityToken,
		RegistryToken: options.RegistryToken,
	}

	resp, err := cli.post(ctx, "/auth", url.Values{}, auth, nil)
	defer ensureReaderClosed(resp)

	if err != nil {
		return RegistryLoginResult{}, err
	}

	var response registry.AuthenticateOKBody
	err = json.NewDecoder(resp.Body).Decode(&response)
	return RegistryLoginResult{Auth: response}, err
}
Repository: moby/moby
File: client/login.go (Go)

Code before:
package client

import (
	"context"
	"encoding/json"
	"net/url"

	"github.com/moby/moby/api/types/registry"
)

// RegistryLogin authenticates the docker server with a given docker registry.
// It returns unauthorizedError when the authentication fails.
func (cli *Client) RegistryLogin(ctx context.Context, auth registry.AuthConfig) (registry.AuthenticateOKBody, error) {
	resp, err := cli.post(ctx, "/auth", url.Values{}, auth, nil)
	defer ensureReaderClosed(resp)

	if err != nil {
		return registry.AuthenticateOKBody{}, err
	}

	var response registry.AuthenticateOKBody
	err = json.NewDecoder(resp.Body).Decode(&response)
	return response, err
}


Review comment (suggestion):
@Benehiko wondering if you've had time to look if all of these fields should indeed be user-configurable as part of the request (mostly looking at `IdentityToken` and `RegistryToken`), or if one of them is only used internally (as part of the registry client round-trip)

Code after:
	"github.com/moby/moby/api/types/registry"
)

type RegistryLoginOptions struct {
	Username      string
	Password      string
	ServerAddress string
	IdentityToken string
	RegistryToken string
}

// RegistryLoginResult holds the result of a RegistryLogin query.
type RegistryLoginResult struct {
	Auth registry.AuthenticateOKBody
}

// RegistryLogin authenticates the docker server with a given docker registry.
// It returns unauthorizedError when the authentication fails.
func (cli *Client) RegistryLogin(ctx context.Context, options RegistryLoginOptions) (RegistryLoginResult, error) {
	auth := registry.AuthConfig{
		Username:      options.Username,
		Password:      options.Password,
		ServerAddress: options.ServerAddress,
		IdentityToken: options.IdentityToken,
		RegistryToken: options.RegistryToken,
	}

	resp, err := cli.post(ctx, "/auth", url.Values{}, auth, nil)
	defer ensureReaderClosed(resp)

	if err != nil {
		return RegistryLoginResult{}, err
	}

	var response registry.AuthenticateOKBody
	err = json.NewDecoder(resp.Body).Decode(&response)
	return RegistryLoginResult{Auth: response}, err
}
Repository: moby/moby
File: client/container_copy.go (Go)

Code before:
package client

import (
	"context"
	"encoding/base64"
	"encoding/json"
	"fmt"
	"io"
	"net/http"
	"net/url"
	"path/filepath"
	"strings"

	"github.com/moby/moby/api/types/container"
)

type ContainerStatPathOptions struct{}

type ContainerStatPathResult struct {
	Stat container.PathStat
}

// ContainerStatPath returns stat information about a path inside the container filesystem.
func (cli *Client) ContainerStatPath(ctx context.Context, containerID, path string, _ ContainerStatPathOptions) (ContainerStatPathResult, error) {
	containerID, err := trimID("container", containerID)
	if err != nil {
		return ContainerStatPathResult{}, err
	}

	query := url.Values{}
	query.Set("path", filepath.ToSlash(path)) // Normalize the paths used in the API.

	resp, err := cli.head(ctx, "/containers/"+containerID+"/archive", query, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return ContainerStatPathResult{}, err
	}
	stat, err := getContainerPathStatFromHeader(resp.Header)
	if err != nil {
		return ContainerStatPathResult{}, err
	}
	return ContainerStatPathResult{Stat: stat}, nil

Review comment (suggestion):
Slightly considering if we should roll up `path` in this as well (I think we did for some other ones), but then again, it's a required argument.


And, we should consider providing a better API for these filesystem operations altogether;

- https://github.com/moby/moby/issues/50976

Code after:
package client

import (
	"context"
	"encoding/base64"
	"encoding/json"
	"fmt"
	"io"
	"net/http"
	"net/url"
	"path/filepath"
	"strings"

	"github.com/moby/moby/api/types/container"
)

type ContainerStatPathOptions struct {
	Path string
}

type ContainerStatPathResult struct {
	Stat container.PathStat
}

// ContainerStatPath returns stat information about a path inside the container filesystem.
func (cli *Client) ContainerStatPath(ctx context.Context, containerID string, options ContainerStatPathOptions) (ContainerStatPathResult, error) {
	containerID, err := trimID("container", containerID)
	if err != nil {
		return ContainerStatPathResult{}, err
	}

	query := url.Values{}
	query.Set("path", filepath.ToSlash(options.Path)) // Normalize the paths used in the API.

	resp, err := cli.head(ctx, "/containers/"+containerID+"/archive", query, nil)
	defer ensureReaderClosed(resp)
	if err != nil {
		return ContainerStatPathResult{}, err
	}
	stat, err := getContainerPathStatFromHeader(resp.Header)
	if err != nil {
		return ContainerStatPathResult{}, err