Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the rebasable flag logic in case of missing label #1866

Merged
merged 4 commits into from
Aug 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"remote_info": null,
"local_info": {
"stack": "pack.test.stack",
"rebasable": false,
"rebasable": {{.rebasable}},
"base_image": {
"top_layer": "{{.base_image_top_layer}}",
"reference": "{{.base_image_id}}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ image_name = "{{.image_name}}"

[local_info]
stack = "pack.test.stack"
rebasable = false
rebasable = {{.rebasable}}

[local_info.base_image]
top_layer = "{{.base_image_top_layer}}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ image_name: "{{.image_name}}"
remote_info:
local_info:
stack: pack.test.stack
rebasable: false
rebasable: {{.rebasable}}
base_image:
top_layer: "{{.base_image_top_layer}}"
reference: "{{.base_image_id}}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"local_info": null,
"remote_info": {
"stack": "pack.test.stack",
"rebasable": false,
"rebasable": {{.rebasable}},
"base_image": {
"top_layer": "{{.base_image_top_layer}}",
"reference": "{{.base_image_ref}}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ image_name = "{{.image_name}}"

[remote_info]
stack = "pack.test.stack"
rebasable = false
rebasable = {{.rebasable}}

[remote_info.base_image]
top_layer = "{{.base_image_top_layer}}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ image_name: "{{.image_name}}"
local_info: null
remote_info:
stack: pack.test.stack
rebasable: false
rebasable: {{.rebasable}}
base_image:
top_layer: "{{.base_image_top_layer}}"
reference: "{{.base_image_ref}}"
Expand Down
18 changes: 16 additions & 2 deletions pkg/client/inspect_image.go
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,8 @@ func (c *Client) InspectImage(name string, daemon bool) (*ImageInfo, error) {
return nil, err
}

var rebasable bool
if _, err := dist.GetLabel(img, platform.RebasableLabel, &rebasable); err != nil {
rebasable, err := getRebasableLabel(img)
if err != nil {
return nil, err
}

Expand Down Expand Up @@ -215,3 +215,17 @@ func (c *Client) InspectImage(name string, daemon bool) (*ImageInfo, error) {
Rebasable: rebasable,
}, nil
}

func getRebasableLabel(labeled dist.Labeled) (bool, error) {
var rebasableOutput bool
isPresent, err := dist.GetLabel(labeled, platform.RebasableLabel, &rebasableOutput)
if err != nil {
return false, err
}

if !isPresent && err == nil {
rebasableOutput = true
}

return rebasableOutput, nil
}
88 changes: 78 additions & 10 deletions pkg/client/inspect_image_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,15 @@ var ignorePlatformAPI = []cmp.Option{

func testInspectImage(t *testing.T, when spec.G, it spec.S) {
var (
subject *Client
mockImageFetcher *testmocks.MockImageFetcher
mockDockerClient *testmocks.MockCommonAPIClient
mockController *gomock.Controller
mockImage *testmocks.MockImage
mockImageNoRebasable *testmocks.MockImage
mockImageWithExtension *testmocks.MockImage
out bytes.Buffer
subject *Client
mockImageFetcher *testmocks.MockImageFetcher
mockDockerClient *testmocks.MockCommonAPIClient
mockController *gomock.Controller
mockImage *testmocks.MockImage
mockImageNoRebasable *testmocks.MockImage
mockImageRebasableWithoutLabel *testmocks.MockImage
mockImageWithExtension *testmocks.MockImage
out bytes.Buffer
)

it.Before(func() {
Expand Down Expand Up @@ -138,7 +139,66 @@ func testInspectImage(t *testing.T, when spec.G, it spec.S) {
}
}`,
))
h.AssertNil(t, mockImage.SetLabel(
h.AssertNil(t, mockImageNoRebasable.SetLabel(
"io.buildpacks.build.metadata",
`{
"bom": [
{
"name": "some-bom-element"
}
],
"buildpacks": [
{
"id": "some-buildpack",
"version": "some-version"
},
{
"id": "other-buildpack",
"version": "other-version"
}
],
"processes": [
{
"type": "other-process",
"command": "/other/process",
"args": ["opt", "1"],
"direct": true
},
{
"type": "web",
"command": "/start/web-process",
"args": ["-p", "1234"],
"direct": false
}
],
"launcher": {
"version": "0.5.0"
}
}`,
))

mockImageRebasableWithoutLabel = testmocks.NewImage("some/imageRebasableWithoutLabel", "", nil)
h.AssertNil(t, mockImageNoRebasable.SetWorkingDir("/test-workdir"))
h.AssertNil(t, mockImageNoRebasable.SetLabel("io.buildpacks.stack.id", "test.stack.id"))
h.AssertNil(t, mockImageNoRebasable.SetLabel(
"io.buildpacks.lifecycle.metadata",
`{
"stack": {
"runImage": {
"image": "some-run-image-no-rebasable",
"mirrors": [
"some-mirror",
"other-mirror"
]
}
},
"runImage": {
"topLayer": "some-top-layer",
"reference": "some-run-image-reference"
}
}`,
))
h.AssertNil(t, mockImageNoRebasable.SetLabel(
"io.buildpacks.build.metadata",
`{
"bom": [
Expand Down Expand Up @@ -259,10 +319,12 @@ func testInspectImage(t *testing.T, when spec.G, it spec.S) {
if useDaemon {
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/image", image.FetchOptions{Daemon: true, PullPolicy: image.PullNever}).Return(mockImage, nil).AnyTimes()
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/imageNoRebasable", image.FetchOptions{Daemon: true, PullPolicy: image.PullNever}).Return(mockImageNoRebasable, nil).AnyTimes()
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/imageRebasableWithoutLabel", image.FetchOptions{Daemon: true, PullPolicy: image.PullNever}).Return(mockImageRebasableWithoutLabel, nil).AnyTimes()
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/imageWithExtension", image.FetchOptions{Daemon: true, PullPolicy: image.PullNever}).Return(mockImageWithExtension, nil).AnyTimes()
} else {
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/image", image.FetchOptions{Daemon: false, PullPolicy: image.PullNever}).Return(mockImage, nil).AnyTimes()
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/imageNoRebasable", image.FetchOptions{Daemon: false, PullPolicy: image.PullNever}).Return(mockImageNoRebasable, nil).AnyTimes()
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/imageRebasableWithoutLabel", image.FetchOptions{Daemon: false, PullPolicy: image.PullNever}).Return(mockImageRebasableWithoutLabel, nil).AnyTimes()
mockImageFetcher.EXPECT().Fetch(gomock.Any(), "some/imageWithExtension", image.FetchOptions{Daemon: false, PullPolicy: image.PullNever}).Return(mockImageWithExtension, nil).AnyTimes()
}
})
Expand Down Expand Up @@ -356,6 +418,12 @@ func testInspectImage(t *testing.T, when spec.G, it spec.S) {
h.AssertEq(t, info.Rebasable, true)
})

it("returns the rebasable image true if the label has not been set", func() {
info, err := subject.InspectImage("some/imageRebasableWithoutLabel", useDaemon)
h.AssertNil(t, err)
h.AssertEq(t, info.Rebasable, true)
})

it("returns the no rebasable image", func() {
info, err := subject.InspectImage("some/imageNoRebasable", useDaemon)
h.AssertNil(t, err)
Expand Down Expand Up @@ -894,7 +962,7 @@ func testInspectImage(t *testing.T, when spec.G, it spec.S) {
Return(fakes.NewImage("missing/labels", "", nil), nil)
info, err := subject.InspectImage("missing/labels", true)
h.AssertNil(t, err)
h.AssertEq(t, info, &ImageInfo{}, ignorePlatformAPI...)
h.AssertEq(t, info, &ImageInfo{Rebasable: true}, ignorePlatformAPI...)
})
})

Expand Down
59 changes: 59 additions & 0 deletions pkg/dist/image_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
package dist_test

import (
"testing"

"github.com/heroku/color"
"github.com/pkg/errors"
"github.com/sclevine/spec"
"github.com/sclevine/spec/report"

"github.com/buildpacks/pack/internal/builder/fakes"
"github.com/buildpacks/pack/pkg/dist"
h "github.com/buildpacks/pack/testhelpers"
)

func TestImage(t *testing.T) {
color.Disable(true)
defer color.Disable(false)
spec.Run(t, "testImage", testImage, spec.Parallel(), spec.Report(report.Terminal{}))
}

func testImage(t *testing.T, when spec.G, it spec.S) {
when("A label needs to be get", func() {
it("sets a label successfully", func() {
var outputLabel bool
mockInspectable := fakes.FakeInspectable{ReturnForLabel: "true", ErrorForLabel: nil}

isPresent, err := dist.GetLabel(&mockInspectable, "random-label", &outputLabel)

h.AssertNil(t, err)
h.AssertEq(t, isPresent, true)
h.AssertEq(t, outputLabel, true)
})

it("returns an error", func() {
var outputLabel bool
mockInspectable := fakes.FakeInspectable{ReturnForLabel: "", ErrorForLabel: errors.New("random-error")}

isPresent, err := dist.GetLabel(&mockInspectable, "random-label", &outputLabel)

h.AssertNotNil(t, err)
h.AssertEq(t, isPresent, false)
h.AssertEq(t, outputLabel, false)
})
})

when("Try to get an empty label", func() {
it("returns isPresent but it doesn't set the label", func() {
var outputLabel bool
mockInspectable := fakes.FakeInspectable{ReturnForLabel: "", ErrorForLabel: nil}

isPresent, err := dist.GetLabel(&mockInspectable, "random-label", &outputLabel)

h.AssertNil(t, err)
h.AssertEq(t, isPresent, false)
h.AssertEq(t, outputLabel, false)
})
})
}