Skip to content

Commit

Permalink
Polishing some code after rebasing to the latest code from pack manif…
Browse files Browse the repository at this point in the history 
…est command PR

Signed-off-by: Juan Bustamante <jbustamante@vmware.com>
  • Loading branch information
@jjbustamante
jjbustamante committed May 6, 2024
1 parent d501165 commit 9c8a2ee
Show file tree
Hide file tree
Showing 6 changed files with 30 additions and 33 deletions.
5 changes: 1 addition & 4 deletions pkg/buildpack/downloader.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,7 @@ type DownloadOptions struct {
// The kind of module to download (valid values: "buildpack", "extension"). Defaults to "buildpack".
ModuleKind string

Daemon bool
Multiarch bool
Daemon bool

PullPolicy image.PullPolicy

Expand Down Expand Up @@ -114,7 +113,6 @@ func (c *buildpackDownloader) Download(ctx context.Context, moduleURI string, op
PullPolicy: opts.PullPolicy,
Platform: opts.Platform,
Target: opts.Target,
MultiArch: opts.Multiarch,
})
if err != nil {
return nil, nil, errors.Wrapf(err, "extracting from registry %s", style.Symbol(moduleURI))
Expand All @@ -131,7 +129,6 @@ func (c *buildpackDownloader) Download(ctx context.Context, moduleURI string, op
PullPolicy: opts.PullPolicy,
Platform: opts.Platform,
Target: opts.Target,
MultiArch: opts.Multiarch,
})
if err != nil {
return nil, nil, errors.Wrapf(err, "extracting from registry %s", style.Symbol(moduleURI))
Expand Down
30 changes: 16 additions & 14 deletions pkg/client/create_builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"github.com/Masterminds/semver"
"github.com/buildpacks/imgutil"
"github.com/google/go-containerregistry/pkg/v1/types"
"github.com/pkg/errors"
"golang.org/x/text/cases"
"golang.org/x/text/language"
Expand Down Expand Up @@ -84,16 +85,16 @@ func (c *Client) CreateBuilder(ctx context.Context, opts CreateBuilderOptions) e

var digests []string
for _, target := range targets {
bldr, err := c.createBaseBuilder(ctx, opts, target, multiArch)
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, multiArch); err != nil {
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, false); err != nil {
if err := c.addExtensionsToBuilder(ctx, opts, bldr); err != nil {
return errors.Wrap(err, "failed to add extensions to builder")
}

Expand Down Expand Up @@ -123,9 +124,11 @@ func (c *Client) CreateBuilder(ctx context.Context, opts CreateBuilderOptions) e

if multiArch && len(digests) > 1 {
return c.CreateManifest(ctx, CreateManifestOptions{
ManifestName: opts.BuilderName,
Manifests: digests,
Publish: true,
IndexRepoName: opts.BuilderName,
RepoNames: digests,
Publish: true,
// TODO: If we do not specify the media type we get an error that can't be empty.
Format: types.OCIImageIndex,
})
}

Expand Down Expand Up @@ -193,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, multiarch bool) (*builder.Builder, error) {
baseImage, err := c.imageFetcher.Fetch(ctx, opts.Config.Build.Image, image.FetchOptions{Daemon: !opts.Publish, PullPolicy: opts.PullPolicy, Platform: fmt.Sprintf("%s/%s", target.OS, target.Arch), Target: &target, MultiArch: multiarch})
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, Platform: fmt.Sprintf("%s/%s", target.OS, target.Arch), Target: &target})
if err != nil {
return nil, errors.Wrap(err, "fetch build image")
}
Expand Down Expand Up @@ -289,25 +292,25 @@ func (c *Client) fetchLifecycle(ctx context.Context, config pubbldr.LifecycleCon
return lifecycle, nil
}

func (c *Client) addBuildpacksToBuilder(ctx context.Context, opts CreateBuilderOptions, bldr *builder.Builder, multiarch bool) error {
func (c *Client) addBuildpacksToBuilder(ctx context.Context, opts CreateBuilderOptions, bldr *builder.Builder) error {
for _, b := range opts.Config.Buildpacks {
if err := c.addConfig(ctx, buildpack.KindBuildpack, b, opts, bldr, multiarch); err != nil {
if err := c.addConfig(ctx, buildpack.KindBuildpack, b, opts, bldr); err != nil {
return err
}
}
return nil
}

func (c *Client) addExtensionsToBuilder(ctx context.Context, opts CreateBuilderOptions, bldr *builder.Builder, multiarch bool) error {
func (c *Client) addExtensionsToBuilder(ctx context.Context, opts CreateBuilderOptions, bldr *builder.Builder) error {
for _, e := range opts.Config.Extensions {
if err := c.addConfig(ctx, buildpack.KindExtension, e, opts, bldr, multiarch); err != nil {
if err := c.addConfig(ctx, buildpack.KindExtension, e, opts, bldr); err != nil {
return err
}
}
return nil
}

func (c *Client) addConfig(ctx context.Context, kind string, config pubbldr.ModuleConfig, opts CreateBuilderOptions, bldr *builder.Builder, multiarch bool) error {
func (c *Client) addConfig(ctx context.Context, kind string, config pubbldr.ModuleConfig, opts CreateBuilderOptions, bldr *builder.Builder) error {
c.logger.Debugf("Looking up %s %s", kind, style.Symbol(config.DisplayString()))

builderOS, err := bldr.Image().OS()
Expand All @@ -332,7 +335,6 @@ func (c *Client) addConfig(ctx context.Context, kind string, config pubbldr.Modu
RegistryName: opts.Registry,
RelativeBaseDir: opts.RelativeBaseDir,
Target: &dist.Target{OS: builderOS, Arch: builderArch},
Multiarch: multiarch,
})
if err != nil {
return errors.Wrapf(err, "downloading %s", kind)
Expand Down
4 changes: 2 additions & 2 deletions pkg/client/create_builder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ 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, Platform: platform, Target: &target, MultiArch: false}).Return(fakeBuildImage, nil)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/build-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Platform: platform, Target: &target}).Return(fakeBuildImage, nil)
h.AssertNil(t, fakeBuildImage.SetLabel("io.buildpacks.stack.id", "other.stack.id"))
prepareFetcherWithRunImages()

Expand Down Expand Up @@ -371,7 +371,7 @@ 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, Platform: platform, Target: &target, MultiArch: false}).Return(fakeBuildImage, nil)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/build-image", image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Platform: platform, Target: &target}).Return(fakeBuildImage, nil)

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"))
Expand Down
4 changes: 3 additions & 1 deletion pkg/client/package_buildpack.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"os"
"path/filepath"

"github.com/google/go-containerregistry/pkg/v1/types"
"github.com/pkg/errors"

pubbldpkg "github.com/buildpacks/pack/buildpackage"
Expand Down Expand Up @@ -166,7 +167,6 @@ func (c *Client) PackageBuildpack(ctx context.Context, opts PackageBuildpackOpti
Daemon: !opts.Publish,
PullPolicy: opts.PullPolicy,
Target: &target,
Multiarch: multiArch,
})
if err != nil {
return errors.Wrapf(err, "packaging dependencies (uri=%s,image=%s)", style.Symbol(dep.URI), style.Symbol(dep.ImageName))
Expand Down Expand Up @@ -214,6 +214,8 @@ func (c *Client) PackageBuildpack(ctx context.Context, opts PackageBuildpackOpti
IndexRepoName: opts.Name,
RepoNames: digests,
Publish: true,
// TODO: If we do not specify the media type we get an error that can't be empty.
Format: types.OCIImageIndex,
})
}

Expand Down
12 changes: 6 additions & 6 deletions pkg/client/package_buildpack_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -270,11 +270,11 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) {
})

shouldFetchNestedPackage := func(demon bool, pull image.PullPolicy) {
mockImageFetcher.EXPECT().Fetch(gomock.Any(), nestedPackage.Name(), image.FetchOptions{Daemon: demon, PullPolicy: pull, Platform: "linux", Target: &dist.Target{OS: "linux"}, MultiArch: false}).Return(nestedPackage, nil)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), nestedPackage.Name(), image.FetchOptions{Daemon: demon, PullPolicy: pull, Platform: "linux", Target: &dist.Target{OS: "linux"}}).Return(nestedPackage, nil)
}

shouldNotFindNestedPackageWhenCallingImageFetcherWith := func(demon bool, pull image.PullPolicy) {
mockImageFetcher.EXPECT().Fetch(gomock.Any(), nestedPackage.Name(), image.FetchOptions{Daemon: demon, PullPolicy: pull, Platform: "linux", Target: &dist.Target{OS: "linux"}, MultiArch: false}).Return(nil, image.ErrNotFound)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), nestedPackage.Name(), image.FetchOptions{Daemon: demon, PullPolicy: pull, Platform: "linux", Target: &dist.Target{OS: "linux"}}).Return(nil, image.ErrNotFound)
}

shouldCreateLocalPackage := func() imgutil.Image {
Expand Down Expand Up @@ -395,7 +395,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) {
when("nested package is not a valid package", func() {
it("should error", func() {
notPackageImage := fakes.NewImage("not/package", "", nil)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), notPackageImage.Name(), image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Platform: "linux", Target: &dist.Target{OS: "linux"}, MultiArch: false}).Return(notPackageImage, nil)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), notPackageImage.Name(), image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Platform: "linux", Target: &dist.Target{OS: "linux"}}).Return(notPackageImage, nil)

mockDockerClient.EXPECT().Info(context.TODO()).Return(system.Info{OSType: "linux"}, nil).AnyTimes()

Expand Down Expand Up @@ -606,7 +606,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) {
PullPolicy: image.PullAlways,
}))

mockImageFetcher.EXPECT().Fetch(gomock.Any(), nestedPackage.Name(), image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Platform: "linux", Target: &dist.Target{OS: "linux"}, MultiArch: false}).Return(nestedPackage, nil)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), nestedPackage.Name(), image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Platform: "linux", Target: &dist.Target{OS: "linux"}}).Return(nestedPackage, nil)
})

it("should pull and use local nested package image", func() {
Expand Down Expand Up @@ -721,7 +721,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) {
PullPolicy: image.PullAlways,
}))

mockImageFetcher.EXPECT().Fetch(gomock.Any(), nestedPackage.Name(), image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Platform: "linux", Target: &dist.Target{OS: "linux"}, MultiArch: false}).Return(nestedPackage, nil)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), nestedPackage.Name(), image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Platform: "linux", Target: &dist.Target{OS: "linux"}}).Return(nestedPackage, nil)
})

it("should include both of them", func() {
Expand Down Expand Up @@ -829,7 +829,7 @@ func testPackageBuildpack(t *testing.T, when spec.G, it spec.S) {
h.AssertNil(t, err)
err = packageImage.SetLabel("io.buildpacks.buildpack.layers", `{"example/foo":{"1.1.0":{"api": "0.2", "layerDiffID":"sha256:xxx", "stacks":[{"id":"some.stack.id"}]}}}`)
h.AssertNil(t, err)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), packageImage.Name(), image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Platform: "linux", Target: &dist.Target{OS: "linux"}, MultiArch: false}).Return(packageImage, nil)
mockImageFetcher.EXPECT().Fetch(gomock.Any(), packageImage.Name(), image.FetchOptions{Daemon: true, PullPolicy: image.PullAlways, Platform: "linux", Target: &dist.Target{OS: "linux"}}).Return(packageImage, nil)

packHome := filepath.Join(tmpDir, "packHome")
h.AssertNil(t, os.Setenv("PACK_HOME", packHome))
Expand Down
8 changes: 2 additions & 6 deletions pkg/image/fetcher.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ type Fetcher struct {

type FetchOptions struct {
Daemon bool
MultiArch bool
Platform string
Target *dist.Target
PullPolicy PullPolicy
Expand Down Expand Up @@ -97,7 +96,7 @@ func (f *Fetcher) Fetch(ctx context.Context, name string, options FetchOptions)
}

if !options.Daemon {
return f.fetchRemoteImage(name, options.Target, options.MultiArch)
return f.fetchRemoteImage(name, options.Target)
}

switch options.PullPolicy {
Expand Down Expand Up @@ -174,17 +173,14 @@ func (f *Fetcher) fetchDaemonImage(name string) (imgutil.Image, error) {
return image, nil
}

func (f *Fetcher) fetchRemoteImage(name string, target *dist.Target, multiarch bool) (imgutil.Image, error) {
func (f *Fetcher) fetchRemoteImage(name string, target *dist.Target) (imgutil.Image, error) {
var (
image imgutil.Image
err error
)

if target == nil {
image, err = remote.NewImage(name, f.keychain, remote.FromBaseImage(name))
} else if multiarch {
// TODO remote.SaveWithDigest()
image, err = remote.NewImage(name, f.keychain, remote.FromBaseImage(name), remote.WithDefaultPlatform(imgutil.Platform{OS: target.OS, Architecture: target.Arch}))
} else {
image, err = remote.NewImage(name, f.keychain, remote.FromBaseImage(name), remote.WithDefaultPlatform(imgutil.Platform{OS: target.OS, Architecture: target.Arch}))
}
Expand Down

0 comments on commit 9c8a2ee

Please sign in to comment.