Fixed changes request by @ashutosh-narkar (squash before merge)

* model Error similar to bundle status

Signed-off-by: Torsten Wunderlich <[email protected]>

plugins/bundle/errors.go

@@ -3,6 +3,8 @@ package bundle
 import (
 	"errors"
 	"fmt"
+
+	"github.com/open-policy-agent/opa/download"
 )
 
 // Errors represents a list of errors that occurred during a bundle load enriched by the bundle name.
@@ -21,12 +23,29 @@ func (e Errors) Error() string {
 }
 
 type Error struct {
-	Name string
-	Err  error
+	BundleName string
+	Code       string
+	HTTPCode   int
+	Message    string
+	Err        error
+}
+
+func NewBundleError(bundleName string, cause error) Error {
+	var (
+		httpError download.HTTPError
+	)
+	switch {
+	case cause == nil:
+		return Error{BundleName: bundleName, Code: "", HTTPCode: -1, Message: "", Err: nil}
+	case errors.As(cause, &httpError):
+		return Error{BundleName: bundleName, Code: errCode, HTTPCode: httpError.StatusCode, Message: httpError.Error(), Err: cause}
+	default:
+		return Error{BundleName: bundleName, Code: errCode, HTTPCode: -1, Message: cause.Error(), Err: cause}
+	}
 }
 
 func (e Error) Error() string {
-	return fmt.Sprintf("'%s': %v", e.Name, e.Err)
+	return fmt.Sprintf("Bundle name: %s, Code: %s, HTTPCode: %d, Message: %s", e.BundleName, errCode, e.HTTPCode, e.Message)
 }
 
 func (e Error) Unwrap() error {

plugins/bundle/errors_test.go

@@ -5,16 +5,17 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/open-policy-agent/opa/ast"
 	"github.com/open-policy-agent/opa/download"
 )
 
 func TestErrors(t *testing.T) {
 	errs := Errors{
-		Error{Name: "foo", Err: fmt.Errorf("foo error")},
-		Error{Name: "bar", Err: fmt.Errorf("bar error")},
+		NewBundleError("foo", fmt.Errorf("foo error")),
+		NewBundleError("bar", fmt.Errorf("bar error")),
 	}
 
-	expected := "'foo': foo error\n'bar': bar error"
+	expected := "Bundle name: foo, Code: bundle_error, HTTPCode: -1, Message: foo error\nBundle name: bar, Code: bundle_error, HTTPCode: -1, Message: bar error"
 	result := errs.Error()
 
 	if result != expected {
@@ -23,8 +24,8 @@ func TestErrors(t *testing.T) {
 }
 
 func TestUnwrapSlice(t *testing.T) {
-	fooErr := Error{Name: "foo", Err: fmt.Errorf("foo error")}
-	barErr := Error{Name: "bar", Err: fmt.Errorf("bar error")}
+	fooErr := NewBundleError("foo", fmt.Errorf("foo error"))
+	barErr := NewBundleError("bar", fmt.Errorf("bar error"))
 
 	errs := Errors{fooErr, barErr}
 
@@ -39,34 +40,106 @@ func TestUnwrapSlice(t *testing.T) {
 }
 
 func TestUnwrap(t *testing.T) {
-	fooErr := Error{Name: "foo", Err: download.HTTPError{StatusCode: 500}}
-	barErr := Error{Name: "bar", Err: download.HTTPError{StatusCode: 400}}
+	serverHTTPError := NewBundleError("server", download.HTTPError{StatusCode: 500})
+	clientHTTPError := NewBundleError("client", download.HTTPError{StatusCode: 400})
+	astErrors := ast.Errors{ast.NewError(ast.ParseErr, ast.NewLocation(nil, "foo.rego", 100, 2), "blarg")}
 
-	errs := Errors{fooErr, barErr}
+	errs := Errors{serverHTTPError, clientHTTPError, NewBundleError("ast", astErrors)}
 
 	// unwrap first bundle.Error
 	var bundleError Error
 	if !errors.As(errs, &bundleError) {
 		t.Fatal("failed to unwrap Error")
 	}
-	if bundleError.Error() != fooErr.Error() {
-		t.Fatalf("expected: %v \ngot: %v", fooErr, bundleError)
+	if bundleError.Error() != serverHTTPError.Error() {
+		t.Fatalf("expected: %v \ngot: %v", serverHTTPError, bundleError)
 	}
 
 	// unwrap first HTTPError
 	var httpError download.HTTPError
 	if !errors.As(errs, &httpError) {
 		t.Fatal("failed to unwrap Error")
 	}
-	if httpError.Error() != fooErr.Err.Error() {
-		t.Fatalf("expected: %v \ngot: %v", fooErr.Err, httpError)
+	if httpError.Error() != serverHTTPError.Err.Error() {
+		t.Fatalf("expected: %v \ngot: %v", serverHTTPError.Err, httpError)
 	}
 
 	// unwrap HTTPError from bundle.Error
 	if !errors.As(bundleError, &httpError) {
 		t.Fatal("failed to unwrap HTTPError")
 	}
-	if httpError.Error() != fooErr.Err.Error() {
-		t.Fatalf("expected: %v \nbgot: %v", fooErr.Err, httpError)
+	if httpError.Error() != serverHTTPError.Err.Error() {
+		t.Fatalf("expected: %v \nbgot: %v", serverHTTPError.Err, httpError)
+	}
+
+	var unwrappedAstErrors ast.Errors
+	if !errors.As(errs, &unwrappedAstErrors) {
+		t.Fatal("failed to unwrap ast.Errors")
+	}
+	if unwrappedAstErrors.Error() != astErrors.Error() {
+		t.Fatalf("expected: %v \ngot: %v", astErrors, unwrappedAstErrors)
+	}
+}
+
+func TestHTTPErrorWrapping(t *testing.T) {
+	err := download.HTTPError{StatusCode: 500}
+	bundleErr := NewBundleError("foo", err)
+
+	if bundleErr.BundleName != "foo" {
+		t.Fatalf("BundleName: expected: %v \ngot: %v", "foo", bundleErr.BundleName)
+	}
+	if bundleErr.HTTPCode != err.StatusCode {
+		t.Fatalf("HTTPCode: expected: %v \ngot: %v", err.StatusCode, bundleErr.HTTPCode)
+	}
+	if bundleErr.Message != err.Error() {
+		t.Fatalf("Message: expected: %v \ngot: %v", err.Error(), bundleErr.Message)
+	}
+	if bundleErr.Code != errCode {
+		t.Fatalf("Code: expected: %v \ngot: %v", errCode, bundleErr.Code)
+	}
+	if bundleErr.Err != err {
+		t.Fatalf("Err: expected: %v \ngot: %v", err, bundleErr.Err)
+	}
+}
+
+func TestASTErrorsWrapping(t *testing.T) {
+	err := ast.Errors{ast.NewError(ast.ParseErr, ast.NewLocation(nil, "foo.rego", 100, 2), "blarg")}
+	bundleErr := NewBundleError("foo", err)
+
+	if bundleErr.BundleName != "foo" {
+		t.Fatalf("BundleName: expected: %v \ngot: %v", "foo", bundleErr.BundleName)
+	}
+	if bundleErr.HTTPCode != -1 {
+		t.Fatalf("HTTPCode: expected: %v \ngot: %v", -1, bundleErr.HTTPCode)
+	}
+	if bundleErr.Message != err.Error() {
+		t.Fatalf("Message: expected: %v \ngot: %v", err.Error(), bundleErr.Message)
+	}
+	if bundleErr.Code != errCode {
+		t.Fatalf("Code: expected: %v \ngot: %v", errCode, bundleErr.Code)
+	}
+	if bundleErr.Err.Error() != err.Error() {
+		t.Fatalf("Err: expected: %v \ngot: %v", err.Error(), bundleErr.Err.Error())
+	}
+}
+
+func TestGenericErrorWrapping(t *testing.T) {
+	err := fmt.Errorf("foo error")
+	bundleErr := NewBundleError("foo", err)
+
+	if bundleErr.BundleName != "foo" {
+		t.Fatalf("BundleName: expected: %v \ngot: %v", "foo", bundleErr.BundleName)
+	}
+	if bundleErr.HTTPCode != -1 {
+		t.Fatalf("HTTPCode: expected: %v \ngot: %v", -1, bundleErr.HTTPCode)
+	}
+	if bundleErr.Message != err.Error() {
+		t.Fatalf("Message: expected: %v \ngot: %v", err.Error(), bundleErr.Message)
+	}
+	if bundleErr.Code != errCode {
+		t.Fatalf("Code: expected: %v \ngot: %v", errCode, bundleErr.Code)
+	}
+	if bundleErr.Err.Error() != err.Error() {
+		t.Fatalf("Err: expected: %v \ngot: %v", err.Error(), bundleErr.Err.Error())
 	}
 }

plugins/bundle/plugin.go

@@ -271,7 +271,7 @@ func (p *Plugin) Trigger(ctx context.Context) error {
 		err := d.Trigger(ctx)
 		// only return errors for TriggerMode manual as periodic bundles will be retried
 		if err != nil && *p.config.Bundles[name].Trigger == plugins.TriggerManual {
-			errs = append(errs, Error{Name: name, Err: err})
+			errs = append(errs, NewBundleError(name, err))
 		}
 	}
 	if len(errs) > 0 {

plugins/bundle/plugin_test.go

@@ -6575,7 +6575,7 @@ func TestPluginManualTriggerWithServerError(t *testing.T) {
 			t.Fatalf("expected exactly one error, got %d", len(bundleErrors))
 		}
 		for _, e := range bundleErrors {
-			if e.Name != "manual" {
+			if e.BundleName != "manual" {
 				t.Fatalf("expect error only for bundle with manual trigger mode")
 			}
 		}