Skip to content

Conversation

@raggi
Copy link
Member

@raggi raggi commented Jul 10, 2025

Volumes are the conventional way to specify state directories.

@raggi raggi requested a review from irbekrm July 10, 2025 22:15
img, err = mutate.Config(img, v1.Config{
Cmd: args,
Cmd: args,
Volumes: bp.volumes,
Copy link
Member

Choose a reason for hiding this comment

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

This part shouldn't be conditional on len(args) > 0 (setting an entrypoint)

I noted this in my upcoming PR to add env var support too.

Copy link
Member

Choose a reason for hiding this comment

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

I'll handle this fix in my upcoming PR.

log.Fatal("at least one of --files or --gopaths must be set")
}
var vols map[string]struct{}
for vol := range strings.SplitSeq(*volumes, ",") {
Copy link
Member

Choose a reason for hiding this comment

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

this requires Go 1.24. But the go.mod still says 1.23.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed go.mod in #31

Volumes are the conventional way to specify state directories.
Copilot AI review requested due to automatic review settings January 5, 2026 16:32
@bradfitz bradfitz merged commit e1b53cd into main Jan 5, 2026
5 checks passed
@bradfitz bradfitz deleted the raggi/volumes branch January 5, 2026 16:33
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds support for specifying OCI image volumes through a new --volumes command-line flag. Volumes provide a conventional way to declare state directories in container images.

Key Changes:

  • Added volumes field to the buildParams struct to store volume definitions
  • Implemented command-line flag parsing to accept comma-separated volume paths
  • Integrated volumes into the OCI image configuration during the build process
Comments suppressed due to low confidence (1)

mkctr.go:325

  • Volumes are only being set when command args are present (inside the if args := flag.Args(); len(args) > 0 block). This means that if no command arguments are provided, the volumes will not be applied to the image even if the --volumes flag was specified.

Consider moving the volumes configuration outside of the args conditional, or include volumes in the Config regardless of whether args are present. Volumes are typically independent of the command specification.

		if args := flag.Args(); len(args) > 0 {
			img, err = mutate.Config(img, v1.Config{
				Cmd:     args,
				Volumes: bp.volumes,
			})
			if err != nil {
				return err
			}
		}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +143 to +146
if vols == nil {
vols = make(map[string]struct{})
}
vols[strings.TrimSpace(vol)] = struct{}{}
Copy link

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

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

The volume parsing logic doesn't handle empty strings correctly. When the volumes flag is empty (default value ""), strings.SplitSeq will still yield one iteration with an empty string, which will be added to the map after TrimSpace. This results in an empty volume key being added to the volumes map.

Consider adding a check to skip empty strings after trimming, or check if the volumes string is empty before attempting to parse it.

Suggested change
if vols == nil {
vols = make(map[string]struct{})
}
vols[strings.TrimSpace(vol)] = struct{}{}
vol = strings.TrimSpace(vol)
if vol == "" {
continue
}
if vols == nil {
vols = make(map[string]struct{})
}
vols[vol] = struct{}{}

Copilot uses AI. Check for mistakes.
bradfitz added a commit that referenced this pull request Jan 5, 2026
This adds support for setting environment variables and fixes two problems
with volume support (merged in #27):

* if no volumes were specified, it added a bogus "" (empty string) volume
  which made docker fail to run the container
* if no args were given, volumes were omitted

This also pulls out some common code into a new withPlatformPrefix helper,
and removes some log spam by silently skipping over "unknown" OS layers
in the source image when discovering what OSes to build for.

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz added a commit that referenced this pull request Jan 5, 2026
This adds support for setting environment variables and fixes two problems
with volume support (merged in #27):

* if no volumes were specified, it added a bogus "" (empty string) volume
  which made docker fail to run the container
* if no args were given, volumes were omitted

This also pulls out some common code into a new withPlatformPrefix helper,
and removes some log spam by silently skipping over "unknown" OS layers
in the source image when discovering what OSes to build for.

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
tomhjp added a commit to tailscale/tailscale that referenced this pull request Jan 8, 2026
Brings in tailscale/mkctr#27.

Updates tailscale/corp#32085

Change-Id: I90160ed1cdc47118ac8fd0712d63a7b590e739d3
Signed-off-by: Tom Proctor <tomhjp@users.noreply.github.com>
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.

2 participants