Skip to content

Commit

Permalink
fixing a little bug when creating a builder without target flag on da…
Browse files Browse the repository at this point in the history 
…rwin

Signed-off-by: Juan Bustamante <jbustamante@vmware.com>
  • Loading branch information
@jjbustamante
jjbustamante committed May 21, 2024
1 parent 3f3b24d commit 68d1efa
Show file tree
Hide file tree
Showing 6 changed files with 86 additions and 78 deletions.
2 changes: 1 addition & 1 deletion pkg/buildpack/multi_architecture_helper.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func (m *MultiArchConfig) Targets() []dist.Target {

// CopyConfigFiles will, given a base directory (which is expected to be the root folder of a single buildpack),
// copy the buildpack.toml file from the base directory into the corresponding platform root folder for each target.
// It will return an array with all the platform root folders where the buildpack.toml file were copied.
// It will return an array with all the platform root folders where the buildpack.toml file was copied.
func (m *MultiArchConfig) CopyConfigFiles(baseDir string) ([]string, error) {
var filesToClean []string
targets := dist.ExpandTargetsDistributions(m.Targets()...)
Expand Down
2 changes: 1 addition & 1 deletion pkg/client/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ func (f *imageFactory) NewImage(repoName string, daemon bool, target dist.Target
return local.NewImage(repoName, f.dockerClient, local.WithDefaultPlatform(platform))
}

// when targeting a registry, we need to use variant and osVersion to hit the correct image
// when targeting a registry, we need to use variant if available to hit the correct image
platform.Variant = target.ArchVariant
if len(target.Distributions) > 0 {
// We assume the given target's distributions were already expanded, we should be dealing with just 1 distribution name and version.
Expand Down
117 changes: 66 additions & 51 deletions pkg/client/create_builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package client
import (
"context"
"fmt"
"runtime"
"sort"
"strings"

Expand Down Expand Up @@ -64,63 +63,80 @@ func (c *Client) CreateBuilder(ctx context.Context, opts CreateBuilderOptions) e
if err != nil {
return err
}
multiArch := len(targets) > 1 && opts.Publish

var digests []string
for _, target := range targets {
if err := c.validateConfig(ctx, opts, target); err != nil {
if len(targets) == 0 {
_, err = c.createBuilderTarget(ctx, opts, nil, false)
if err != nil {
return err
}
} else {
var digests []string
multiArch := len(targets) > 1 && opts.Publish

bldr, err := c.createBaseBuilder(ctx, opts, target)
if err != nil {
return errors.Wrap(err, "failed to create builder")
for _, target := range targets {
digest, err := c.createBuilderTarget(ctx, opts, &target, multiArch)
if err != nil {
return err
}
digests = append(digests, digest)
}

if err := c.addBuildpacksToBuilder(ctx, opts, bldr); err != nil {
return errors.Wrap(err, "failed to add buildpacks to builder")
if multiArch && len(digests) > 1 {
return c.CreateManifest(ctx, CreateManifestOptions{
IndexRepoName: opts.BuilderName,
RepoNames: digests,
Publish: true,
})
}
}

if err := c.addExtensionsToBuilder(ctx, opts, bldr); err != nil {
return errors.Wrap(err, "failed to add extensions to builder")
}
return nil
}

bldr.SetOrder(opts.Config.Order)
bldr.SetOrderExtensions(opts.Config.OrderExtensions)
func (c *Client) createBuilderTarget(ctx context.Context, opts CreateBuilderOptions, target *dist.Target, multiArch bool) (string, error) {
if err := c.validateConfig(ctx, opts, target); err != nil {
return "", err
}

if opts.Config.Stack.ID != "" {
bldr.SetStack(opts.Config.Stack)
}
bldr.SetRunImage(opts.Config.Run)
bldr.SetBuildConfigEnv(opts.BuildConfigEnv)
bldr, err := c.createBaseBuilder(ctx, opts, target)
if err != nil {
return "", errors.Wrap(err, "failed to create builder")
}

err = bldr.Save(c.logger, builder.CreatorMetadata{Version: c.version})
if err != nil {
return err
}
if err := c.addBuildpacksToBuilder(ctx, opts, bldr); err != nil {
return "", errors.Wrap(err, "failed to add buildpacks to builder")
}

if multiArch {
// We need to keep the identifier to create the image index
id, err := bldr.Image().Identifier()
if err != nil {
return errors.Wrapf(err, "determining image manifest digest")
}
digests = append(digests, id.String())
}
if err := c.addExtensionsToBuilder(ctx, opts, bldr); err != nil {
return "", errors.Wrap(err, "failed to add extensions to builder")
}

if multiArch && len(digests) > 1 {
return c.CreateManifest(ctx, CreateManifestOptions{
IndexRepoName: opts.BuilderName,
RepoNames: digests,
Publish: true,
})
bldr.SetOrder(opts.Config.Order)
bldr.SetOrderExtensions(opts.Config.OrderExtensions)

if opts.Config.Stack.ID != "" {
bldr.SetStack(opts.Config.Stack)
}
bldr.SetRunImage(opts.Config.Run)
bldr.SetBuildConfigEnv(opts.BuildConfigEnv)

return nil
err = bldr.Save(c.logger, builder.CreatorMetadata{Version: c.version})
if err != nil {
return "", err
}

if multiArch {
// We need to keep the identifier to create the image index
id, err := bldr.Image().Identifier()
if err != nil {
return "", errors.Wrapf(err, "determining image manifest digest")
}
return id.String(), nil
}
return "", nil
}

func (c *Client) validateConfig(ctx context.Context, opts CreateBuilderOptions, target dist.Target) error {
func (c *Client) validateConfig(ctx context.Context, opts CreateBuilderOptions, target *dist.Target) error {
if err := pubbldr.ValidateConfig(opts.Config); err != nil {
return errors.Wrap(err, "invalid builder config")
}
Expand All @@ -132,12 +148,12 @@ func (c *Client) validateConfig(ctx context.Context, opts CreateBuilderOptions,
return nil
}

func (c *Client) validateRunImageConfig(ctx context.Context, opts CreateBuilderOptions, target dist.Target) error {
func (c *Client) validateRunImageConfig(ctx context.Context, opts CreateBuilderOptions, target *dist.Target) error {
var runImages []imgutil.Image
for _, r := range opts.Config.Run.Images {
for _, i := range append([]string{r.Image}, r.Mirrors...) {
if !opts.Publish {
img, err := c.imageFetcher.Fetch(ctx, i, image.FetchOptions{Daemon: true, PullPolicy: opts.PullPolicy, Target: &target})
img, err := c.imageFetcher.Fetch(ctx, i, image.FetchOptions{Daemon: true, PullPolicy: opts.PullPolicy, Target: target})
if err != nil {
if errors.Cause(err) != image.ErrNotFound {
return errors.Wrap(err, "failed to fetch image")
Expand All @@ -148,7 +164,7 @@ func (c *Client) validateRunImageConfig(ctx context.Context, opts CreateBuilderO
}
}

img, err := c.imageFetcher.Fetch(ctx, i, image.FetchOptions{Daemon: false, PullPolicy: opts.PullPolicy, Target: &target})
img, err := c.imageFetcher.Fetch(ctx, i, image.FetchOptions{Daemon: false, PullPolicy: opts.PullPolicy, Target: target})
if err != nil {
if errors.Cause(err) != image.ErrNotFound {
return errors.Wrap(err, "failed to fetch image")
Expand Down Expand Up @@ -181,8 +197,8 @@ func (c *Client) validateRunImageConfig(ctx context.Context, opts CreateBuilderO
return nil
}

func (c *Client) createBaseBuilder(ctx context.Context, opts CreateBuilderOptions, target dist.Target) (*builder.Builder, error) {
baseImage, err := c.imageFetcher.Fetch(ctx, opts.Config.Build.Image, image.FetchOptions{Daemon: !opts.Publish, PullPolicy: opts.PullPolicy, Target: &target})
func (c *Client) createBaseBuilder(ctx context.Context, opts CreateBuilderOptions, target *dist.Target) (*builder.Builder, error) {
baseImage, err := c.imageFetcher.Fetch(ctx, opts.Config.Build.Image, image.FetchOptions{Daemon: !opts.Publish, PullPolicy: opts.PullPolicy, Target: target})
if err != nil {
return nil, errors.Wrap(err, "fetch build image")
}
Expand Down Expand Up @@ -363,19 +379,18 @@ func (c *Client) addConfig(ctx context.Context, kind string, config pubbldr.Modu

func (c *Client) processBuilderCreateTargets(ctx context.Context, opts CreateBuilderOptions) ([]dist.Target, error) {
var targets []dist.Target

if len(opts.Targets) > 0 {
// when exporting to the daemon, we need to select just one target
if !opts.Publish {
if opts.Publish {
targets = opts.Targets
} else {
// find a target that matches the daemon
daemonTarget, err := c.daemonTarget(ctx, opts.Targets)
if err != nil {
return targets, err
}
targets = append(targets, daemonTarget)
} else {
targets = opts.Targets
}
} else {
targets = append(targets, dist.Target{OS: runtime.GOOS, Arch: runtime.GOARCH})
}
return targets, nil
}
Expand Down
39 changes: 16 additions & 23 deletions pkg/client/create_builder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"fmt"
"os"
"path/filepath"
"runtime"
"strings"
"testing"

Expand Down Expand Up @@ -59,7 +58,6 @@ func testCreateBuilder(t *testing.T, when spec.G, it spec.S) {
logger logging.Logger
out bytes.Buffer
tmpDir string
target dist.Target
)
var prepareFetcherWithRunImages = func() {
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/run-image", gomock.Any()).Return(fakeRunImage, nil).AnyTimes()
Expand Down Expand Up @@ -188,11 +186,6 @@ func testCreateBuilder(t *testing.T, when spec.G, it spec.S) {

tmpDir, err = os.MkdirTemp("", "create-builder-test")
h.AssertNil(t, err)

target = dist.Target{
OS: runtime.GOOS,
Arch: runtime.GOARCH,
}
})

it.After(func() {
Expand Down Expand Up @@ -225,8 +218,8 @@ func testCreateBuilder(t *testing.T, when spec.G, it spec.S) {
})

it("should fail when the stack ID from the builder config does not match the stack ID from the build image", func() {
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/build-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Target: &target}).Return(fakeBuildImage, nil)
h.AssertNil(t, fakeBuildImage.SetLabel("io.buildpacks.stack.id", "other.stack.id"))
prepareFetcherWithBuildImage()
prepareFetcherWithRunImages()

err := subject.CreateBuilder(context.TODO(), opts)
Expand Down Expand Up @@ -369,13 +362,13 @@ func testCreateBuilder(t *testing.T, when spec.G, it spec.S) {
})

it("should warn when the run image cannot be found", func() {
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/build-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Target: &target}).Return(fakeBuildImage, nil)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/build-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways}).Return(fakeBuildImage, nil)

mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/run-image", image.FetchOptions{Daemon: false, PullPolicy: image.PullAlways, Target: &target}).Return(nil, errors.Wrap(image.ErrNotFound, "yikes"))
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/run-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Target: &target}).Return(nil, errors.Wrap(image.ErrNotFound, "yikes"))
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/run-image", image.FetchOptions{Daemon: false, PullPolicy: image.PullAlways}).Return(nil, errors.Wrap(image.ErrNotFound, "yikes"))
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/run-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways}).Return(nil, errors.Wrap(image.ErrNotFound, "yikes"))

mockImageFetcher.EXPECT().Fetch(gomock.Any(), "localhost:5000/some/run-image", image.FetchOptions{Daemon: false, PullPolicy: image.PullAlways, Target: &target}).Return(nil, errors.Wrap(image.ErrNotFound, "yikes"))
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "localhost:5000/some/run-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Target: &target}).Return(nil, errors.Wrap(image.ErrNotFound, "yikes"))
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "localhost:5000/some/run-image", image.FetchOptions{Daemon: false, PullPolicy: image.PullAlways}).Return(nil, errors.Wrap(image.ErrNotFound, "yikes"))
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "localhost:5000/some/run-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways}).Return(nil, errors.Wrap(image.ErrNotFound, "yikes"))

err := subject.CreateBuilder(context.TODO(), opts)
h.AssertNil(t, err)
Expand All @@ -384,14 +377,14 @@ func testCreateBuilder(t *testing.T, when spec.G, it spec.S) {
})

it("should fail when not publish and the run image cannot be fetched", func() {
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/run-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Target: &target}).Return(nil, errors.New("yikes"))
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/run-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways}).Return(nil, errors.New("yikes"))

err := subject.CreateBuilder(context.TODO(), opts)
h.AssertError(t, err, "failed to fetch image: yikes")
})

it("should fail when publish and the run image cannot be fetched", func() {
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/run-image", image.FetchOptions{Daemon: false, PullPolicy: image.PullAlways, Target: &target}).Return(nil, errors.New("yikes"))
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/run-image", image.FetchOptions{Daemon: false, PullPolicy: image.PullAlways}).Return(nil, errors.New("yikes"))

opts.Publish = true
err := subject.CreateBuilder(context.TODO(), opts)
Expand All @@ -411,12 +404,12 @@ func testCreateBuilder(t *testing.T, when spec.G, it spec.S) {
when("publish is true", func() {
it("should only try to validate the remote run image", func() {
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/build-image", image.FetchOptions{Daemon: true}).Times(0)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/run-image", image.FetchOptions{Daemon: true, Target: &target}).Times(0)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/run-image", image.FetchOptions{Daemon: true}).Times(0)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "localhost:5000/some/run-image", image.FetchOptions{Daemon: true}).Times(0)

mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/build-image", image.FetchOptions{Daemon: false, Target: &target}).Return(fakeBuildImage, nil)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/run-image", image.FetchOptions{Daemon: false, Target: &target}).Return(fakeRunImage, nil)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "localhost:5000/some/run-image", image.FetchOptions{Daemon: false, Target: &target}).Return(fakeRunImageMirror, nil)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/build-image", image.FetchOptions{Daemon: false}).Return(fakeBuildImage, nil)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/run-image", image.FetchOptions{Daemon: false}).Return(fakeRunImage, nil)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "localhost:5000/some/run-image", image.FetchOptions{Daemon: false}).Return(fakeRunImageMirror, nil)

opts.Publish = true

Expand All @@ -430,7 +423,7 @@ func testCreateBuilder(t *testing.T, when spec.G, it spec.S) {
when("build image not found", func() {
it("should fail", func() {
prepareFetcherWithRunImages()
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/build-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Target: &target}).Return(nil, image.ErrNotFound)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/build-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways}).Return(nil, image.ErrNotFound)

err := subject.CreateBuilder(context.TODO(), opts)
h.AssertError(t, err, "fetch build image: not found")
Expand All @@ -442,7 +435,7 @@ func testCreateBuilder(t *testing.T, when spec.G, it spec.S) {
fakeImage := fakeBadImageStruct{}

prepareFetcherWithRunImages()
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/build-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Target: &target}).Return(fakeImage, nil)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/build-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways}).Return(fakeImage, nil)

err := subject.CreateBuilder(context.TODO(), opts)
h.AssertError(t, err, "failed to create builder: invalid build-image")
Expand All @@ -466,7 +459,7 @@ func testCreateBuilder(t *testing.T, when spec.G, it spec.S) {
prepareFetcherWithRunImages()

h.AssertNil(t, fakeBuildImage.SetOS("windows"))
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/build-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Target: &target}).Return(fakeBuildImage, nil)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/build-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways}).Return(fakeBuildImage, nil)

err = packClientWithExperimental.CreateBuilder(context.TODO(), opts)
h.AssertNil(t, err)
Expand All @@ -478,7 +471,7 @@ func testCreateBuilder(t *testing.T, when spec.G, it spec.S) {
prepareFetcherWithRunImages()

h.AssertNil(t, fakeBuildImage.SetOS("windows"))
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/build-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Target: &target}).Return(fakeBuildImage, nil)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/build-image", gomock.Any()).Return(fakeBuildImage, nil)

err := subject.CreateBuilder(context.TODO(), opts)
h.AssertError(t, err, "failed to create builder: Windows containers support is currently experimental.")
Expand Down
2 changes: 1 addition & 1 deletion pkg/client/package_buildpack.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,12 +262,12 @@ func (c *Client) validateOSPlatform(ctx context.Context, os string, publish bool
return nil
}

// daemonTarget returns a target that matches with the given daemon os/arch
func (c *Client) daemonTarget(ctx context.Context, targets []dist.Target) (dist.Target, error) {
info, err := c.docker.ServerVersion(ctx)
if err != nil {
return dist.Target{}, err
}

for _, t := range targets {
if t.Arch != "" && t.OS == info.Os && t.Arch == info.Arch {
return t, nil
Expand Down
2 changes: 1 addition & 1 deletion pkg/image/fetcher.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ func (f *Fetcher) fetchRemoteImage(name string, target *dist.Target) (imgutil.Im
image, err = remote.NewImage(name, f.keychain, remote.FromBaseImage(name))
} else {
platform := imgutil.Platform{OS: target.OS, Architecture: target.Arch}
// when targeting a registry, we need to use variant and osVersion to hit the correct image
// when targeting a registry, we need to use variant if available to hit the correct image
platform.Variant = target.ArchVariant
if len(target.Distributions) > 0 {
// We assumed the given target's distributions were expanded, and we are just dealing with 1 version.
Expand Down

0 comments on commit 68d1efa

Please sign in to comment.