From 9c8a2eef283853abf9ffa4494b6077c05a8d57c5 Mon Sep 17 00:00:00 2001 From: Juan Bustamante Date: Mon, 6 May 2024 10:31:10 -0500 Subject: [PATCH] Polishing some code after rebasing to the latest code from pack manifest command PR Signed-off-by: Juan Bustamante --- pkg/buildpack/downloader.go | 5 +---- pkg/client/create_builder.go | 30 +++++++++++++++------------- pkg/client/create_builder_test.go | 4 ++-- pkg/client/package_buildpack.go | 4 +++- pkg/client/package_buildpack_test.go | 12 +++++------ pkg/image/fetcher.go | 8 ++------ 6 files changed, 30 insertions(+), 33 deletions(-) diff --git a/pkg/buildpack/downloader.go b/pkg/buildpack/downloader.go index c08eae9990..bb471c7b67 100644 --- a/pkg/buildpack/downloader.go +++ b/pkg/buildpack/downloader.go @@ -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 @@ -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)) @@ -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)) diff --git a/pkg/client/create_builder.go b/pkg/client/create_builder.go index d49f719e0a..75dfb9fa41 100644 --- a/pkg/client/create_builder.go +++ b/pkg/client/create_builder.go @@ -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" @@ -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") } @@ -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, }) } @@ -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") } @@ -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() @@ -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) diff --git a/pkg/client/create_builder_test.go b/pkg/client/create_builder_test.go index c972f36119..e6918ae385 100644 --- a/pkg/client/create_builder_test.go +++ b/pkg/client/create_builder_test.go @@ -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() @@ -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")) diff --git a/pkg/client/package_buildpack.go b/pkg/client/package_buildpack.go index a543d681fd..f3dd08d11f 100644 --- a/pkg/client/package_buildpack.go +++ b/pkg/client/package_buildpack.go @@ -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" @@ -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)) @@ -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, }) } diff --git a/pkg/client/package_buildpack_test.go b/pkg/client/package_buildpack_test.go index 1206250226..c2207c83e5 100644 --- a/pkg/client/package_buildpack_test.go +++ b/pkg/client/package_buildpack_test.go @@ -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 { @@ -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() @@ -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() { @@ -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() { @@ -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)) diff --git a/pkg/image/fetcher.go b/pkg/image/fetcher.go index 635801137c..89e38614e2 100644 --- a/pkg/image/fetcher.go +++ b/pkg/image/fetcher.go @@ -63,7 +63,6 @@ type Fetcher struct { type FetchOptions struct { Daemon bool - MultiArch bool Platform string Target *dist.Target PullPolicy PullPolicy @@ -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 { @@ -174,7 +173,7 @@ 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 @@ -182,9 +181,6 @@ func (f *Fetcher) fetchRemoteImage(name string, target *dist.Target, multiarch b 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})) }