From b0933b5d3ec1e3c6102c0e7c872c2bad4342672f Mon Sep 17 00:00:00 2001
From: Javier Romero <jromero@pivotal.io>
Date: Wed, 9 Oct 2019 10:16:28 -0500
Subject: [PATCH] Display proper error when buildpack.toml is missing required
 fields

Signed-off-by: Javier Romero <jromero@pivotal.io>
---
 create_builder.go      |  2 +-
 dist/buildpack.go      | 23 ++++++++++++++++++---
 dist/buildpack_test.go | 45 +++++++++++++++++++++++++++++++++++++++---
 3 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/create_builder.go b/create_builder.go
index cebc121ae0..741fbee4ba 100644
--- a/create_builder.go
+++ b/create_builder.go
@@ -73,7 +73,7 @@ func (c *Client) CreateBuilder(ctx context.Context, opts CreateBuilderOptions) e
 
 		fetchedBp, err := dist.NewBuildpack(blob)
 		if err != nil {
-			return errors.Wrap(err, "creating buildpack")
+			return errors.Wrapf(err, "creating buildpack from %s", style.Symbol(b.URI))
 		}
 
 		err = validateBuildpack(fetchedBp, b.URI, b.ID, b.Version)
diff --git a/dist/buildpack.go b/dist/buildpack.go
index 060ebe2000..6d83872bb0 100644
--- a/dist/buildpack.go
+++ b/dist/buildpack.go
@@ -1,7 +1,6 @@
 package dist
 
 import (
-	"fmt"
 	"io"
 	"strings"
 
@@ -78,12 +77,30 @@ func NewBuildpack(blob Blob) (Buildpack, error) {
 }
 
 func validateDescriptor(bpd BuildpackDescriptor) error {
+	if bpd.Info.ID == "" {
+		return errors.Errorf("%s is required", style.Symbol("buildpack.id"))
+	}
+
+	if bpd.Info.Version == "" {
+		return errors.Errorf("%s is required", style.Symbol("buildpack.version"))
+	}
+
 	if len(bpd.Order) == 0 && len(bpd.Stacks) == 0 {
-		return fmt.Errorf("buildpack %s must have either stacks or an order defined", style.Symbol(bpd.Info.ID+"@"+bpd.Info.Version))
+		return errors.Errorf(
+			"buildpack %s: must have either %s or an %s defined",
+			style.Symbol(bpd.Info.ID+"@"+bpd.Info.Version),
+			style.Symbol("stacks"),
+			style.Symbol("order"),
+		)
 	}
 
 	if len(bpd.Order) >= 1 && len(bpd.Stacks) >= 1 {
-		return fmt.Errorf("buildpack %s cannot have both stacks and an order defined", style.Symbol(bpd.Info.ID+"@"+bpd.Info.Version))
+		return errors.Errorf(
+			"buildpack %s: cannot have both %s and an %s defined",
+			style.Symbol(bpd.Info.ID+"@"+bpd.Info.Version),
+			style.Symbol("stacks"),
+			style.Symbol("order"),
+		)
 	}
 
 	return nil
diff --git a/dist/buildpack_test.go b/dist/buildpack_test.go
index 2c3138d553..c7144747d8 100644
--- a/dist/buildpack_test.go
+++ b/dist/buildpack_test.go
@@ -63,7 +63,7 @@ id = "some.stack.id"
 		})
 
 		when("there is no api field", func() {
-			it("assumes a version", func() {
+			it.Before(func() {
 				h.AssertNil(t, ioutil.WriteFile(filepath.Join(tmpBpDir, "buildpack.toml"), []byte(`
 [buildpack]
 id = "bp.one"
@@ -72,13 +72,52 @@ version = "1.2.3"
 [[stacks]]
 id = "some.stack.id"
 `), os.ModePerm))
+			})
 
+			it("assumes an api version", func() {
 				bp, err := dist.NewBuildpack(blob.NewBlob(tmpBpDir))
 				h.AssertNil(t, err)
 				h.AssertEq(t, bp.Descriptor().API.String(), "0.1")
 			})
 		})
 
+		when("there is no id", func() {
+			it.Before(func() {
+				h.AssertNil(t, ioutil.WriteFile(filepath.Join(tmpBpDir, "buildpack.toml"), []byte(`
+[buildpack]
+id = ""
+version = "1.2.3"
+
+[[stacks]]
+id = "some.stack.id"
+`), os.ModePerm))
+			})
+
+			it("returns error", func() {
+				_, err := dist.NewBuildpack(blob.NewBlob(tmpBpDir))
+				h.AssertError(t, err, "'buildpack.id' is required")
+			})
+		})
+
+		when("there is no version", func() {
+			it.Before(func() {
+				h.AssertNil(t, ioutil.WriteFile(filepath.Join(tmpBpDir, "buildpack.toml"), []byte(`
+[buildpack]
+id = "bp.one"
+version = ""
+
+[[stacks]]
+id = "some.stack.id"
+`), os.ModePerm))
+			})
+
+			it("returns error", func() {
+				_, err := dist.NewBuildpack(blob.NewBlob(tmpBpDir))
+				h.AssertError(t, err, "'buildpack.version' is required")
+
+			})
+		})
+
 		when("both stacks and order are present", func() {
 			it.Before(func() {
 				h.AssertNil(t, ioutil.WriteFile(filepath.Join(tmpBpDir, "buildpack.toml"), []byte(`
@@ -98,7 +137,7 @@ id = "some.stack.id"
 
 			it("returns error", func() {
 				_, err := dist.NewBuildpack(blob.NewBlob(tmpBpDir))
-				h.AssertError(t, err, "cannot have both stacks and an order defined")
+				h.AssertError(t, err, "cannot have both 'stacks' and an 'order' defined")
 			})
 		})
 
@@ -113,7 +152,7 @@ version = "1.2.3"
 
 			it("returns error", func() {
 				_, err := dist.NewBuildpack(blob.NewBlob(tmpBpDir))
-				h.AssertError(t, err, "must have either stacks or an order defined")
+				h.AssertError(t, err, "must have either 'stacks' or an 'order' defined")
 			})
 		})
 	})