internal/action: changed ErrNotAllowed to variable for correct errors…

….Is() comparison

Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>

cmd/opm/alpha/diff/cmd.go

@@ -47,8 +47,8 @@ declarative config containing only packages channels, and versions not present
 in the old set, and versions that differ between the old and new sets. This is known as "latest" mode.
 
 These references are passed through 'opm render' to produce a single declarative config.
-Bundle image refs are not supported directly;
-a valid "olm.package" declarative config object referring to the bundle's package must exist in all input refs.
+Bundle image refs are not supported directly; a valid "olm.package" declarative config object
+referring to the bundle's package must exist in all input refs.
 
 This command has special behavior when old-refs are omitted, called "heads-only" mode:
 instead of the output being that of 'opm render refs...'

internal/action/diff.go

@@ -35,8 +35,8 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) {
 		oldRender := Render{Refs: a.OldRefs, Registry: a.Registry, AllowedRefMask: mask}
 		oldCfg, err := oldRender.Run(ctx)
 		if err != nil {
-			if isNotAllowedError(err) {
-				return nil, fmt.Errorf("%v (diff does not permit direct bundle references)", err)
+			if errors.Is(err, ErrNotAllowed) {
+				return nil, fmt.Errorf("%w (diff does not permit direct bundle references)", err)
 			}
 			return nil, fmt.Errorf("error rendering old refs: %v", err)
 		}
@@ -49,8 +49,8 @@ func (a Diff) Run(ctx context.Context) (*declcfg.DeclarativeConfig, error) {
 	newRender := Render{Refs: a.NewRefs, Registry: a.Registry, AllowedRefMask: mask}
 	newCfg, err := newRender.Run(ctx)
 	if err != nil {
-		if isNotAllowedError(err) {
-			return nil, fmt.Errorf("%v (diff does not permit direct bundle references)", err)
+		if errors.Is(err, ErrNotAllowed) {
+			return nil, fmt.Errorf("%w (diff does not permit direct bundle references)", err)
 		}
 		return nil, fmt.Errorf("error rendering new refs: %v", err)
 	}
@@ -74,8 +74,3 @@ func (p Diff) validate() error {
 	}
 	return nil
 }
-
-func isNotAllowedError(err error) bool {
-	errNotAllowed := &ErrNotAllowed{}
-	return errors.As(err, &errNotAllowed)
-}

internal/action/diff_test.go

@@ -3,12 +3,14 @@ package action_test
 import (
 	"context"
 	"embed"
+	"errors"
 	"io/fs"
 	"path"
 	"path/filepath"
 	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/operator-framework/operator-registry/internal/action"
@@ -49,6 +51,37 @@ func TestDiff(t *testing.T) {
 			expectedCfg: loadDirFS(t, indicesDir, filepath.Join("testdata", "index-declcfgs", "exp-headsonly")),
 			assertion:   require.NoError,
 		},
+		{
+			name: "Fail/NewBundleImage",
+			diff: action.Diff{
+				Registry: registry,
+				NewRefs:  []string{"test.registry/foo-operator/foo-bundle:v0.1.0"},
+			},
+			assertion: func(t require.TestingT, err error, _ ...interface{}) {
+				if !assert.Error(t, err) {
+					require.Fail(t, "expected an error")
+				}
+				if !errors.Is(err, action.ErrNotAllowed) {
+					require.Fail(t, "err is not action.ErrNotAllowed", err)
+				}
+			},
+		},
+		{
+			name: "Fail/OldBundleImage",
+			diff: action.Diff{
+				Registry: registry,
+				OldRefs:  []string{"test.registry/foo-operator/foo-bundle:v0.1.0"},
+				NewRefs:  []string{filepath.Join("testdata", "index-declcfgs", "latest")},
+			},
+			assertion: func(t require.TestingT, err error, _ ...interface{}) {
+				if !assert.Error(t, err) {
+					require.Fail(t, "expected an error")
+				}
+				if !errors.Is(err, action.ErrNotAllowed) {
+					require.Fail(t, "err is not action.ErrNotAllowed", err)
+				}
+			},
+		},
 	}
 
 	for _, s := range specs {

internal/action/list.go

@@ -206,7 +206,7 @@ func indexRefToModel(ctx context.Context, ref string) (model.Model, error) {
 	}
 	cfg, err := render.Run(ctx)
 	if err != nil {
-		if errors.Is(err, &ErrNotAllowed{}) {
+		if errors.Is(err, ErrNotAllowed) {
 			return nil, fmt.Errorf("cannot list non-index %q", ref)
 		}
 		return nil, err

internal/action/render.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"database/sql"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -41,13 +42,7 @@ func (r RefType) Allowed(refType RefType) bool {
 	return r == RefAll || r&refType == refType
 }
 
-var _ error = &ErrNotAllowed{}
-
-type ErrNotAllowed struct{}
-
-func (_ *ErrNotAllowed) Error() string {
-	return "not allowed"
-}
+var ErrNotAllowed = errors.New("not allowed")
 
 type Render struct {
 	Refs           []string
@@ -108,7 +103,7 @@ func (r Render) renderReference(ctx context.Context, ref string) (*declcfg.Decla
 	if stat, serr := os.Stat(ref); serr == nil {
 		if stat.IsDir() {
 			if !r.AllowedRefMask.Allowed(RefDCDir) {
-				return nil, fmt.Errorf("cannot render DC directory: %w", &ErrNotAllowed{})
+				return nil, fmt.Errorf("cannot render declarative config directory: %w", ErrNotAllowed)
 			}
 			return declcfg.LoadFS(os.DirFS(ref))
 		} else {
@@ -118,7 +113,7 @@ func (r Render) renderReference(ctx context.Context, ref string) (*declcfg.Decla
 				return nil, err
 			}
 			if !r.AllowedRefMask.Allowed(RefSqliteFile) {
-				return nil, fmt.Errorf("cannot render sqlite file: %w", &ErrNotAllowed{})
+				return nil, fmt.Errorf("cannot render sqlite file: %w", ErrNotAllowed)
 			}
 			return sqliteToDeclcfg(ctx, ref)
 		}
@@ -147,23 +142,23 @@ func (r Render) imageToDeclcfg(ctx context.Context, imageRef string) (*declcfg.D
 	var cfg *declcfg.DeclarativeConfig
 	if dbFile, ok := labels[containertools.DbLocationLabel]; ok {
 		if !r.AllowedRefMask.Allowed(RefSqliteImage) {
-			return nil, fmt.Errorf("cannot render sqlite image: %w", &ErrNotAllowed{})
+			return nil, fmt.Errorf("cannot render sqlite image: %w", ErrNotAllowed)
 		}
 		cfg, err = sqliteToDeclcfg(ctx, filepath.Join(tmpDir, dbFile))
 		if err != nil {
 			return nil, err
 		}
 	} else if configsDir, ok := labels[containertools.ConfigsLocationLabel]; ok {
 		if !r.AllowedRefMask.Allowed(RefDCImage) {
-			return nil, fmt.Errorf("cannot render DC image: %w", &ErrNotAllowed{})
+			return nil, fmt.Errorf("cannot render declarative config image: %w", ErrNotAllowed)
 		}
 		cfg, err = declcfg.LoadFS(os.DirFS(filepath.Join(tmpDir, configsDir)))
 		if err != nil {
 			return nil, err
 		}
 	} else if _, ok := labels[bundle.PackageLabel]; ok {
 		if !r.AllowedRefMask.Allowed(RefBundleImage) {
-			return nil, fmt.Errorf("cannot render bundle image: %w", &ErrNotAllowed{})
+			return nil, fmt.Errorf("cannot render bundle image: %w", ErrNotAllowed)
 		}
 		img, err := registry.NewImageInput(ref, tmpDir)
 		if err != nil {

internal/action/render_test.go

@@ -448,7 +448,7 @@ func TestAllowRefMask(t *testing.T) {
 				Registry:       reg,
 				AllowedRefMask: action.RefDCImage | action.RefDCDir | action.RefSqliteFile | action.RefBundleImage,
 			},
-			expectErr: &action.ErrNotAllowed{},
+			expectErr: action.ErrNotAllowed,
 		},
 		{
 			name: "SqliteFile/Allowed",
@@ -466,7 +466,7 @@ func TestAllowRefMask(t *testing.T) {
 				Registry:       reg,
 				AllowedRefMask: action.RefDCImage | action.RefDCDir | action.RefSqliteImage | action.RefBundleImage,
 			},
-			expectErr: &action.ErrNotAllowed{},
+			expectErr: action.ErrNotAllowed,
 		},
 		{
 			name: "DeclcfgImage/Allowed",
@@ -484,7 +484,7 @@ func TestAllowRefMask(t *testing.T) {
 				Registry:       reg,
 				AllowedRefMask: action.RefDCDir | action.RefSqliteImage | action.RefSqliteFile | action.RefBundleImage,
 			},
-			expectErr: &action.ErrNotAllowed{},
+			expectErr: action.ErrNotAllowed,
 		},
 		{
 			name: "DeclcfgDir/Allowed",
@@ -502,7 +502,7 @@ func TestAllowRefMask(t *testing.T) {
 				Registry:       reg,
 				AllowedRefMask: action.RefDCImage | action.RefSqliteImage | action.RefSqliteFile | action.RefBundleImage,
 			},
-			expectErr: &action.ErrNotAllowed{},
+			expectErr: action.ErrNotAllowed,
 		},
 		{
 			name: "BundleImage/Allowed",
@@ -520,7 +520,7 @@ func TestAllowRefMask(t *testing.T) {
 				Registry:       reg,
 				AllowedRefMask: action.RefDCImage | action.RefDCDir | action.RefSqliteImage | action.RefSqliteFile,
 			},
-			expectErr: &action.ErrNotAllowed{},
+			expectErr: action.ErrNotAllowed,
 		},
 		{
 			name: "All/Allowed",