Skip to content

Commit

Permalink
Simplify pkg/dockerfile interface by ditching pointer
Browse files Browse the repository at this point in the history
This means slightly more typing in "zero-value" cases (`nil` vs `dockerfile.Metadata{}`), but the tradeoff is that it's simpler to use and reason about (and all the struct members are pointer-type map/slice values anyhow, so copying the struct is still pretty cheap).

This also swaps the scanner error handling to return the partially parsed Metadata object alongside the scanner error -- the error already tells us the object isn't fully complete data, so it's fair/fine to return and will likely just be ignored by the caller instead.  This also allows us to get to 100% code coverage. 👀

This also updates our "treat `oci-import` just like `FROM scratch`" code to *actually* parse `FROM scratch` so we can't accidentally cause "missing data" bugs there in the future, and I implemented that using `sync.OnceValues` which requires upgrading to Go 1.21, but IMO that's a worthwhile tradeoff (because `sync.OnceValues` makes that code so clean/simple).
  • Loading branch information
tianon committed Jan 10, 2025
1 parent 7ddf2be commit 60ee93c
Show file tree
Hide file tree
Showing 9 changed files with 26 additions and 34 deletions.
2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FROM golang:1.20-bullseye AS build
FROM golang:1.21-bookworm AS build

SHELL ["bash", "-Eeuo", "pipefail", "-xc"]

Expand Down
2 changes: 1 addition & 1 deletion Dockerfile.release
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FROM golang:1.20-bullseye
FROM golang:1.21-bookworm

SHELL ["bash", "-Eeuo", "pipefail", "-xc"]

Expand Down
2 changes: 1 addition & 1 deletion Dockerfile.test
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FROM golang:1.20-bullseye
FROM golang:1.21-bookworm

SHELL ["bash", "-Eeuo", "pipefail", "-xc"]

Expand Down
6 changes: 3 additions & 3 deletions bashbrew.sh
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ set -Eeuo pipefail
dir="$(readlink -f "$BASH_SOURCE")"
dir="$(dirname "$dir")"

: "${CGO_ENABLED:=0}"
export GO111MODULE=on CGO_ENABLED
: "${CGO_ENABLED=0}" "${GOTOOLCHAIN=local}"
export CGO_ENABLED GOTOOLCHAIN
(
cd "$dir"
go build -o bin/bashbrew ./cmd/bashbrew > /dev/null
go build -trimpath -o bin/bashbrew ./cmd/bashbrew > /dev/null
)

exec "$dir/bin/bashbrew" "$@"
27 changes: 13 additions & 14 deletions cmd/bashbrew/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"os/exec"
"path"
"strings"
"sync"

"github.com/docker-library/bashbrew/manifest"
"github.com/docker-library/bashbrew/pkg/dockerfile"
Expand Down Expand Up @@ -37,27 +38,25 @@ func (r Repo) ArchDockerFroms(arch string, entry *manifest.Manifest2822Entry) ([
return dockerfileMeta.Froms, nil
}

func (r Repo) dockerfileMetadata(entry *manifest.Manifest2822Entry) (*dockerfile.Metadata, error) {
func (r Repo) dockerfileMetadata(entry *manifest.Manifest2822Entry) (dockerfile.Metadata, error) {
return r.archDockerfileMetadata(arch, entry)
}

var dockerfileMetadataCache = map[string]*dockerfile.Metadata{}
var (
dockerfileMetadataCache = map[string]dockerfile.Metadata{}
scratchDockerfileMetadata = sync.OnceValues(func() (dockerfile.Metadata, error) {
return dockerfile.Parse(`FROM scratch`)
})
)

func (r Repo) archDockerfileMetadata(arch string, entry *manifest.Manifest2822Entry) (*dockerfile.Metadata, error) {
func (r Repo) archDockerfileMetadata(arch string, entry *manifest.Manifest2822Entry) (dockerfile.Metadata, error) {
if builder := entry.ArchBuilder(arch); builder == "oci-import" {
return &dockerfile.Metadata{
StageFroms: []string{
"scratch",
},
Froms: []string{
"scratch",
},
}, nil
return scratchDockerfileMetadata()
}

commit, err := r.fetchGitRepo(arch, entry)
if err != nil {
return nil, cli.NewMultiError(fmt.Errorf("failed fetching Git repo for arch %q from entry %q", arch, entry.String()), err)
return dockerfile.Metadata{}, cli.NewMultiError(fmt.Errorf("failed fetching Git repo for arch %q from entry %q", arch, entry.String()), err)
}

dockerfileFile := path.Join(entry.ArchDirectory(arch), entry.ArchFile(arch))
Expand All @@ -72,12 +71,12 @@ func (r Repo) archDockerfileMetadata(arch string, entry *manifest.Manifest2822En

df, err := gitShow(commit, dockerfileFile)
if err != nil {
return nil, cli.NewMultiError(fmt.Errorf(`failed "git show" for %q from commit %q`, dockerfileFile, commit), err)
return dockerfile.Metadata{}, cli.NewMultiError(fmt.Errorf(`failed "git show" for %q from commit %q`, dockerfileFile, commit), err)
}

meta, err := dockerfile.Parse(df)
if err != nil {
return nil, cli.NewMultiError(fmt.Errorf(`failed parsing Dockerfile metadata for %q from commit %q`, dockerfileFile, commit), err)
return dockerfile.Metadata{}, cli.NewMultiError(fmt.Errorf(`failed parsing Dockerfile metadata for %q from commit %q`, dockerfileFile, commit), err)
}

dockerfileMetadataCache[cacheKey] = meta
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module github.com/docker-library/bashbrew

go 1.20
go 1.21

require (
github.com/containerd/containerd v1.6.19
Expand Down
1 change: 1 addition & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,7 @@ github.com/prometheus/procfs v0.1.3/go.mod h1:lV6e/gmhEcM9IjHGsFOCxxuZ+z1YqCvr4O
github.com/prometheus/procfs v0.2.0/go.mod h1:lV6e/gmhEcM9IjHGsFOCxxuZ+z1YqCvr4OA4YeYWdaU=
github.com/prometheus/procfs v0.6.0/go.mod h1:cz+aTbrPOrUb4q7XlbU9ygM+/jj0fzG6c1xBZuNvfVA=
github.com/prometheus/procfs v0.7.3 h1:4jVXhlkAyzOScmCkXBTOLRLTz8EeU+eyjrwB/EPq0VU=
github.com/prometheus/procfs v0.7.3/go.mod h1:cz+aTbrPOrUb4q7XlbU9ygM+/jj0fzG6c1xBZuNvfVA=
github.com/prometheus/tsdb v0.7.1/go.mod h1:qhTCs0VvXwvX/y3TZrWD7rabWM+ijKTux40TwIPHuXU=
github.com/rogpeppe/fastuuid v0.0.0-20150106093220-6724a57986af/go.mod h1:XWv6SoW27p1b0cqNHllgS5HIMJraePCO15w5zCzIWYg=
github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ=
Expand Down
11 changes: 4 additions & 7 deletions pkg/dockerfile/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@ type Metadata struct {
Froms []string // every "FROM" or "COPY --from=xxx" value (minus named and/or numbered stages in the case of "--from=")
}

func Parse(dockerfile string) (*Metadata, error) {
func Parse(dockerfile string) (Metadata, error) {
return ParseReader(strings.NewReader(dockerfile))
}

func ParseReader(dockerfile io.Reader) (*Metadata, error) {
meta := &Metadata{
func ParseReader(dockerfile io.Reader) (Metadata, error) {
meta := Metadata{
// panic: assignment to entry in nil map
StageNameFroms: map[string]string{},
// (nil slices work fine)
Expand Down Expand Up @@ -171,10 +171,7 @@ func ParseReader(dockerfile io.Reader) (*Metadata, error) {
}
}
}
if err := scanner.Err(); err != nil {
return nil, err
}
return meta, nil
return meta, scanner.Err()
}

func latestizeRepoTag(repoTag string) string {
Expand Down
7 changes: 1 addition & 6 deletions pkg/dockerfile/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,14 +175,9 @@ func TestParse(t *testing.T) {
if err != nil {
t.Fatal(err)
}

if parsed == nil {
if !reflect.DeepEqual(parsed, td.metadata) {
t.Fatalf("expected:\n%#v\ngot:\n%#v", td.metadata, parsed)
}

if !reflect.DeepEqual(*parsed, td.metadata) {
t.Fatalf("expected:\n%#v\ngot:\n%#v", td.metadata, *parsed)
}
})
}
}

0 comments on commit 60ee93c

Please sign in to comment.