FIXME: pass the logger and print the warning inside (#1038)

* initial changes in layermetadata.go for FIXME Signed-off-by: anushkamittal20 <anumittal4641@gmail.com> * minor changes, var name and added TODO Signed-off-by: anushkamittal20 <anumittal4641@gmail.com> * Added check for empty string in layermetadata.go Signed-off-by: anushkamittal20 <anumittal4641@gmail.com> * checks in layermetadata_test.go for logger Signed-off-by: anushkamittal20 <anumittal4641@gmail.com> * api version check - logic moved Signed-off-by: anushkamittal20 <anumittal4641@gmail.com> * ran make format for modified files Signed-off-by: anushkamittal20 <anumittal4641@gmail.com> * removed comments Signed-off-by: anushkamittal20 <anumittal4641@gmail.com> * changes to DecodeLayerMetadataFile Signed-off-by: anushkamittal20 <anumittal4641@gmail.com> * check for version shifted from layers.go to layermetadata.go Signed-off-by: anushkamittal20 <anumittal4641@gmail.com> * resolved ineffectual assignment to err Signed-off-by: anushkamittal20 <anumittal4641@gmail.com> * added new it block, TODO resolve failing test Signed-off-by: anushkamittal20 <anumittal4641@gmail.com> * Fix failing test Signed-off-by: anushkamittal20 <anumittal4641@gmail.com> * removed unnecessary fmt Signed-off-by: anushkamittal20 <anumittal4641@gmail.com> --------- Signed-off-by: anushkamittal20 <anumittal4641@gmail.com>
2023-05-30 20:37:39 +05:30 · 2023-05-30 20:37:39 +05:30 · 5dbedd3ca0
parent 5c29f544ea
commit 5dbedd3ca0
4 changed files with 53 additions and 23 deletions
--- a/buildpack/build.go
+++ b/buildpack/build.go
@ -164,24 +164,18 @@ func runBuildCmd(d BpDescriptor, bpLayersDir, planPath string, inputs BuildInput
 func (d BpDescriptor) processLayers(layersDir string, logger log.Logger) (map[string]LayerMetadataFile, error) {
 	if api.MustParse(d.WithAPI).LessThan("0.6") {
 		return eachLayer(layersDir, d.WithAPI, func(path, buildpackAPI string) (LayerMetadataFile, error) {
-			layerMetadataFile, msg, err := DecodeLayerMetadataFile(path+".toml", buildpackAPI)
+			layerMetadataFile, err := DecodeLayerMetadataFile(path+".toml", buildpackAPI, logger)
 			if err != nil {
 				return LayerMetadataFile{}, err
 			}
-			if msg != "" {
-				logger.Warn(msg)
-			}
 			return layerMetadataFile, nil
 		})
 	}
 	return eachLayer(layersDir, d.WithAPI, func(path, buildpackAPI string) (LayerMetadataFile, error) {
-		layerMetadataFile, msg, err := DecodeLayerMetadataFile(path+".toml", buildpackAPI)
+		layerMetadataFile, err := DecodeLayerMetadataFile(path+".toml", buildpackAPI, logger)
 		if err != nil {
 			return LayerMetadataFile{}, err
 		}
-		if msg != "" {
-			return LayerMetadataFile{}, errors.New(msg)
-		}
 		if err := renameLayerDirIfNeeded(layerMetadataFile, path); err != nil {
 			return LayerMetadataFile{}, err
 		}
--- a/buildpack/layermetadata.go
+++ b/buildpack/layermetadata.go
@ -7,6 +7,8 @@ import (

 	"github.com/BurntSushi/toml"

+	"github.com/buildpacks/lifecycle/log"
+
 	"github.com/buildpacks/lifecycle/api"
 )

@ -34,12 +36,12 @@ func EncodeLayerMetadataFile(lmf LayerMetadataFile, path, buildpackAPI string) e
 	return errors.New("couldn't find an encoder")
 }

-func DecodeLayerMetadataFile(path, buildpackAPI string) (LayerMetadataFile, string, error) { // FIXME: pass the logger and print the warning inside (instead of returning a message)
+func DecodeLayerMetadataFile(path string, buildpackAPI string, logger log.Logger) (LayerMetadataFile, error) { // FIXME: pass the logger and print the warning inside (instead of returning a message)
 	fh, err := os.Open(path)
 	if os.IsNotExist(err) {
-		return LayerMetadataFile{}, "", nil
+		return LayerMetadataFile{}, nil
 	} else if err != nil {
-		return LayerMetadataFile{}, "", err
+		return LayerMetadataFile{}, err
 	}
 	defer fh.Close()

@ -47,10 +49,18 @@ func DecodeLayerMetadataFile(path, buildpackAPI string) (LayerMetadataFile, stri

 	for _, decoder := range decoders {
 		if decoder.IsSupported(buildpackAPI) {
-			return decoder.Decode(path)
+			lmf, str, err := decoder.Decode(path)
+			if str != "" {
+				if api.MustParse(buildpackAPI).LessThan("0.6") {
+					logger.Warn(str)
+				} else {
+					return LayerMetadataFile{}, errors.New(str)
+				}
+			}
+			return lmf, err
 		}
 	}
-	return LayerMetadataFile{}, "", errors.New("couldn't find a decoder")
+	return LayerMetadataFile{}, errors.New("couldn't find a decoder")
 }

 type encoderDecoder interface {
--- a/buildpack/layermetadata_test.go
+++ b/buildpack/layermetadata_test.go
@ -4,13 +4,22 @@ import (
 	"os"
 	"testing"

+	"github.com/apex/log"
+
+	"github.com/apex/log/handlers/memory"
+
 	"github.com/sclevine/spec"
 	"github.com/sclevine/spec/report"

+	llog "github.com/buildpacks/lifecycle/log"
+
 	"github.com/buildpacks/lifecycle/buildpack"
 	h "github.com/buildpacks/lifecycle/testhelpers"
 )

+var logHandler *memory.Handler
+var logger llog.Logger
+
 func TestLayerMetadata(t *testing.T) {
 	spec.Run(t, "unit-layermetadata", testLayerMetadata, spec.Report(report.Terminal{}))
 }
@ -24,6 +33,8 @@ func testLayerMetadata(t *testing.T, when spec.G, it spec.S) {
 			var err error
 			metadataFile, err = os.CreateTemp("", "test")
 			h.AssertNil(t, err)
+			logHandler = memory.New()
+			logger = &log.Logger{Handler: logHandler}
 		})
 		it.After(func() {
 			os.Remove(metadataFile.Name())
@ -33,12 +44,34 @@ func testLayerMetadata(t *testing.T, when spec.G, it spec.S) {
 			h.AssertNil(t, err)

 			var lmf buildpack.LayerMetadataFile
-			lmf, s, err := buildpack.DecodeLayerMetadataFile(metadataFile.Name(), "0.9")
+			lmf, err = buildpack.DecodeLayerMetadataFile(metadataFile.Name(), "0.9", logger)
 			h.AssertNil(t, err)
-			h.AssertEq(t, s, "")
 			h.AssertEq(t, lmf.Cache, true)
 			h.AssertEq(t, lmf.Build, false)
 			h.AssertEq(t, lmf.Launch, false)
 		})
+		it("logs a warning when the metadata file has wrong format (on older apis)", func() {
+			err := os.WriteFile(metadataFile.Name(), []byte("[types]\ncache = true"), 0400)
+			h.AssertNil(t, err)
+			var lmf buildpack.LayerMetadataFile
+			lmf, err = buildpack.DecodeLayerMetadataFile(metadataFile.Name(), "0.5", logger)
+			h.AssertNil(t, err)
+			h.AssertEq(t, lmf.Cache, false)
+			h.AssertEq(t, lmf.Build, false)
+			h.AssertEq(t, lmf.Launch, false)
+			expected := "Types table isn't supported in this buildpack api version. The launch, build and cache flags should be in the top level. Ignoring the values in the types table."
+			h.AssertLogEntry(t, logHandler, expected)
+		})
+		it("returns an error when the metadata file has wrong format", func() {
+			err := os.WriteFile(metadataFile.Name(), []byte("cache = true"), 0400)
+			h.AssertNil(t, err)
+
+			var lmf buildpack.LayerMetadataFile
+			lmf, err = buildpack.DecodeLayerMetadataFile(metadataFile.Name(), "0.9", logger)
+			h.AssertNotNil(t, err)
+			h.AssertEq(t, lmf.Cache, false)
+			h.AssertEq(t, lmf.Build, false)
+			h.AssertEq(t, lmf.Launch, false)
+		})
 	})
 }
--- a/buildpack/layers.go
+++ b/buildpack/layers.go
@ -148,17 +148,10 @@ func (l *Layer) Path() string {

 func (l *Layer) Read() (LayerMetadata, error) {
 	tomlPath := l.Path() + ".toml"
-	layerMetadataFile, msg, err := DecodeLayerMetadataFile(tomlPath, l.api)
+	layerMetadataFile, err := DecodeLayerMetadataFile(tomlPath, l.api, l.logger)
 	if err != nil {
 		return LayerMetadata{}, err
 	}
-	if msg != "" {
-		if api.MustParse(l.api).LessThan("0.6") {
-			l.logger.Warn(msg)
-		} else {
-			return LayerMetadata{}, errors.New(msg)
-		}
-	}
 	var sha string
 	shaBytes, err := os.ReadFile(l.Path() + ".sha")
 	if err != nil && !os.IsNotExist(err) { // if the sha file doesn't exist, an empty sha will be returned