Skip to content

Commit

Permalink
removing previous attempt to default the Target if user doesn't provi…
Browse files Browse the repository at this point in the history 
…ded one

Signed-off-by: Juan Bustamante <jbustamante@vmware.com>
  • Loading branch information
@jjbustamante
jjbustamante committed May 21, 2024
1 parent dad414b commit 7cca91b
Show file tree
Hide file tree
Showing 8 changed files with 93 additions and 120 deletions.
2 changes: 0 additions & 2 deletions pkg/buildpack/downloader_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (

"github.com/buildpacks/imgutil/fakes"
"github.com/buildpacks/lifecycle/api"
"github.com/docker/docker/api/types"
"github.com/docker/docker/api/types/system"
"github.com/golang/mock/gomock"
"github.com/heroku/color"
Expand Down Expand Up @@ -62,7 +61,6 @@ func testBuildpackDownloader(t *testing.T, when spec.G, it spec.S) {
var createPackage = func(imageName string) *fakes.Image {
packageImage := fakes.NewImage(imageName, "", nil)
mockImageFactory.EXPECT().NewImage(packageImage.Name(), false, dist.Target{OS: "linux"}).Return(packageImage, nil)
mockDockerClient.EXPECT().ServerVersion(gomock.Any()).Return(types.Version{Os: "linux", Arch: "amd64"}, nil)

pack, err := client.NewClient(
client.WithLogger(logger),
Expand Down
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
131 changes: 64 additions & 67 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 @@ -67,46 +66,19 @@ func (c *Client) CreateBuilder(ctx context.Context, opts CreateBuilderOptions) e
multiArch := len(targets) > 1 && opts.Publish

var digests []string
for _, target := range targets {
if err := c.validateConfig(ctx, opts, target); err != nil {
return err
}

bldr, err := c.createBaseBuilder(ctx, opts, target)
if err != nil {
return errors.Wrap(err, "failed to create builder")
}

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

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

bldr.SetOrder(opts.Config.Order)
bldr.SetOrderExtensions(opts.Config.OrderExtensions)

if opts.Config.Stack.ID != "" {
bldr.SetStack(opts.Config.Stack)
for _, t := range targets {
var target *dist.Target
if t.OS == "" && t.Arch == "" {
target = nil
} else {
target = &t
}
bldr.SetRunImage(opts.Config.Run)
bldr.SetBuildConfigEnv(opts.BuildConfigEnv)

err = bldr.Save(c.logger, builder.CreatorMetadata{Version: c.version})
digest, err := c.createBuilderTarget(ctx, opts, target, multiArch)
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")
}
digests = append(digests, id.String())
}
digests = append(digests, digest)
}

if multiArch && len(digests) > 1 {
Expand All @@ -120,7 +92,50 @@ func (c *Client) CreateBuilder(ctx context.Context, opts CreateBuilderOptions) e
return nil
}

func (c *Client) validateConfig(ctx context.Context, opts CreateBuilderOptions, target dist.Target) error {
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
}

bldr, err := c.createBaseBuilder(ctx, opts, target)
if err != nil {
return "", errors.Wrap(err, "failed to create builder")
}

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

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

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)

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 {
if err := pubbldr.ValidateConfig(opts.Config); err != nil {
return errors.Wrap(err, "invalid builder config")
}
Expand All @@ -132,12 +147,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 +163,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 +196,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 @@ -364,37 +379,19 @@ 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

// For backward compatibility, we must create a target with de default platform used in the Fetch implementation for daemon and remote
// When daemon
// OS: daemonInfo.Os,
// Architecture: daemonInfo.Arch
// When remote || layout
// OS: "linux",
// Architecture: runtime.GOARCH

if !opts.Publish {
// daemon option
info, err := c.docker.ServerVersion(ctx)
if err != nil {
return targets, err
}
if len(opts.Targets) > 0 {
// when exporting to the daemon, we need to select just one target
daemonTarget, err := c.daemonTarget(info, opts.Targets)
if len(opts.Targets) > 0 {
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 = append(targets, dist.Target{OS: info.Os, Arch: info.Arch})
}
} else {
// remote option
if len(opts.Targets) > 0 {
targets = opts.Targets
} else {
targets = append(targets, dist.Target{OS: "linux", Arch: runtime.GOARCH})
}
targets = append(targets, dist.Target{})
}
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
13 changes: 6 additions & 7 deletions pkg/client/package_buildpack.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"fmt"
"path/filepath"

"github.com/docker/docker/api/types"
"github.com/pkg/errors"

pubbldpkg "github.com/buildpacks/pack/buildpackage"
Expand Down Expand Up @@ -229,14 +228,10 @@ func (c *Client) downloadBuildpackFromURI(ctx context.Context, uri, relativeBase

func (c *Client) processPackageBuildpackTargets(ctx context.Context, opts PackageBuildpackOptions) ([]dist.Target, error) {
var targets []dist.Target
info, err := c.docker.ServerVersion(ctx)
if err != nil {
return targets, err
}
if len(opts.Targets) > 0 {
// when exporting to the daemon, we need to select just one target
if !opts.Publish && opts.Format == FormatImage {
daemonTarget, err := c.daemonTarget(info, opts.Targets)
daemonTarget, err := c.daemonTarget(ctx, opts.Targets)
if err != nil {
return targets, err
}
Expand Down Expand Up @@ -268,7 +263,11 @@ func (c *Client) validateOSPlatform(ctx context.Context, os string, publish bool
}

// daemonTarget returns a target that matches with the given daemon os/arch
func (c *Client) daemonTarget(info types.Version, targets []dist.Target) (dist.Target, error) {
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
Loading

0 comments on commit 7cca91b

Please sign in to comment.