Merge pull request buildpacks#338 from buildpack/bugfix/312-ambiguous…

…-error-when-bp-version-missing Display proper error when buildpack.toml is missing required fields
mdelillo · Oct 14, 2019 · 74c59d5 · 74c59d5
2 parents 936e971 + b0933b5
commit 74c59d5
Show file tree

Hide file tree

Showing 3 changed files with 63 additions and 7 deletions.
diff --git 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
@@ -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
@@ -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")
 			})
 		})
 	})