Merge pull request #1866 from dlion/1799-fix-rebasable-logic-and-test

Fix the rebasable flag logic in case of missing label

acceptance/testdata/pack_fixtures/inspect_image_local_output.json

@@ -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}}"

acceptance/testdata/pack_fixtures/inspect_image_local_output.toml

@@ -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}}"

acceptance/testdata/pack_fixtures/inspect_image_local_output.yaml

@@ -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}}"

acceptance/testdata/pack_fixtures/inspect_image_published_output.json

@@ -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}}"

acceptance/testdata/pack_fixtures/inspect_image_published_output.toml

@@ -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}}"

acceptance/testdata/pack_fixtures/inspect_image_published_output.yaml

@@ -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}}"

pkg/client/inspect_image.go

@@ -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
 	}
 
@@ -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
+}

pkg/client/inspect_image_test.go

@@ -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() {
@@ -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": [
@@ -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()
 					}
 				})
@@ -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)
@@ -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...)
 		})
 	})
 

pkg/dist/image_test.go

@@ -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)
+		})
+	})
+}