refactor

Signed-off-by: Per Goncalves da Silva <[email protected]>

Author: Per Goncalves da Silva

cmd/manager/main.go

@@ -47,6 +47,7 @@ import (
 	"github.com/operator-framework/rukpak/pkg/storage"
 
 	ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
+	"github.com/operator-framework/operator-controller/internal/action"
 	"github.com/operator-framework/operator-controller/internal/catalogmetadata/cache"
 	catalogclient "github.com/operator-framework/operator-controller/internal/catalogmetadata/client"
 	"github.com/operator-framework/operator-controller/internal/controllers"
@@ -176,9 +177,10 @@ func main() {
 		os.Exit(1)
 	}
 
-	acg, err := helmclient.NewActionClientGetter(cfgGetter,
+	acg, err := action.NewWrappedActionClientGetter(cfgGetter,
 		helmclient.WithFailureRollbacks(false),
 	)
+
 	if err != nil {
 		setupLog.Error(err, "unable to create helm client")
 		os.Exit(1)

go.mod

@@ -14,8 +14,6 @@ require (
 	github.com/operator-framework/helm-operator-plugins v0.3.0
 	github.com/operator-framework/operator-registry v1.44.0
 	github.com/operator-framework/rukpak v0.24.0
-	github.com/pkg/errors v0.9.1
-	github.com/sirupsen/logrus v1.9.3
 	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.9.0
 	go.uber.org/zap v1.27.0
@@ -192,6 +190,7 @@ require (
 	github.com/otiai10/copy v1.14.0 // indirect
 	github.com/peterbourgon/diskv v2.0.1+incompatible // indirect
 	github.com/pjbgf/sha1cd v0.3.0 // indirect
+	github.com/pkg/errors v0.9.1 // indirect
 	github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
 	github.com/prometheus/client_golang v1.19.1 // indirect
 	github.com/prometheus/client_model v0.6.1 // indirect
@@ -202,6 +201,7 @@ require (
 	github.com/russross/blackfriday/v2 v2.1.0 // indirect
 	github.com/sergi/go-diff v1.3.2-0.20230802210424-5b0b94c5c0d3 // indirect
 	github.com/shopspring/decimal v1.3.1 // indirect
+	github.com/sirupsen/logrus v1.9.3 // indirect
 	github.com/skeema/knownhosts v1.2.2 // indirect
 	github.com/spf13/cast v1.5.0 // indirect
 	github.com/spf13/cobra v1.8.1 // indirect

internal/action/error/errors.go

@@ -3,82 +3,75 @@ package error
 import (
 	"fmt"
 	"regexp"
-
-	"github.com/pkg/errors"
-	"github.com/sirupsen/logrus"
 )
 
 const (
-	installConflictErrorPattern        = `Unable to continue with install: (\w+) "(.*)" in namespace "(.*)" exists and cannot be imported into the current release.*`
-	installConflictErrorGeneralPattern = `Unable to continue with install: .*`
+	defaultErrMessage = "An unexpected error prevented the operator from succeeding"
 )
 
-// InstallConflictErr indicates one or more resources already exist in the cluster
-type InstallConflictErr error
+var (
+	installConflictErrorPattern = regexp.MustCompile(`Unable to continue with install: (\w+) "(.*)" in namespace "(.*)" exists and cannot be imported into the current release.*`)
+)
 
-// UnknownErr indicates an error that cannot be classified
-type UnknownErr error
+type Olmv1Err struct {
+	originalErr error
+	message     string
+}
 
-func AsOlmErr(err error) error {
-	if err == nil {
-		return nil
-	}
-	for _, rule := range rules {
-		if matches := rule.MatchError(err); len(matches) > 0 {
-			// first index is the full match
-			logrus.Debugf("found match with params %v\n", matches)
-			return rule.TranslateError(err, matches[1:]...)
-		}
-	}
+func (o Olmv1Err) Error() string {
+	return o.message
+}
 
-	// let's mark any unmatched errors as unknown
-	return UnknownErr(err)
+func (o Olmv1Err) Cause() error {
+	return o.originalErr
 }
 
-// errorTranslator is a function that translates an error into a more specific error
-// typically to hide implementation details
-type errorTranslator func(error, ...string) error
+func newOlmv1Err(originalErr error, message string) error {
+	return &Olmv1Err{
+		originalErr: originalErr,
+		message:     message,
+	}
+}
 
-// errorMatcher is a function that matches an error to a specific pattern and extracts any
-// relevant information - if nil is returned, no match was found
-type errorMatcher func(error) []string
+func AsOlmErr(originalErr error) error {
+	if originalErr == nil {
+		return nil
+	}
 
-// rule links a matcher to a translator
-type rule struct {
-	matchFunc  errorMatcher
-	translator errorTranslator
-}
+	for _, exec := range rules {
+		if err := exec(originalErr); err != nil {
+			return err
+		}
+	}
 
-func (r rule) MatchError(err error) []string {
-	return r.matchFunc(err)
+	// let's mark any unmatched errors as unknown
+	return defaultErrTranslator(originalErr)
 }
 
-func (r rule) TranslateError(err error, params ...string) error {
-	return r.translator(err, params...)
-}
+// rule is a function that translates an error into a more specific error
+// typically to hide internal implementation details
+// in: helm error
+// out: nil -> no translation | !nil -> translated error
+type rule func(originalErr error) error
 
 // rules is a list of rules for error translation
 var rules = []rule{
-	{regexErrorMatcher(installConflictErrorPattern), installConflictErrorTranslator},
-	{regexErrorMatcher(installConflictErrorGeneralPattern), installConflictErrorTranslator},
+	helmInstallConflictErr,
 }
 
-// installConflictErrorTranslator c
-func installConflictErrorTranslator(_ error, params ...string) error {
-	msg := "A conflict prevented the installation from proceeding. Please ensure there are no conflicting resources already present in the cluster."
-	if len(params) == 3 {
-		kind := params[0]
-		name := params[1]
-		namespace := params[2]
-		msg = fmt.Sprintf("%s '%s' already exists in namespace '%s'", kind, name, namespace)
+// installConflictErrorTranslator
+func helmInstallConflictErr(originalErr error) error {
+	matches := installConflictErrorPattern.FindStringSubmatch(originalErr.Error())
+	if len(matches) != 4 {
+		// there was no match
+		return nil
 	}
-	return InstallConflictErr(errors.New(msg))
+	kind := matches[1]
+	name := matches[2]
+	namespace := matches[3]
+	return newOlmv1Err(originalErr, fmt.Sprintf("%s '%s' already exists in namespace '%s'", kind, name, namespace))
 }
 
-// takes a valid regexp and calls FindStringSubmatch to assert match and extract parameters
-func regexErrorMatcher(regex string) errorMatcher {
-	re := regexp.MustCompile(regex)
-	return func(err error) []string {
-		return re.FindStringSubmatch(err.Error())
-	}
+func defaultErrTranslator(originalErr error) error {
+	return newOlmv1Err(originalErr, defaultErrMessage)
 }

internal/action/error/errors_test.go

@@ -1,7 +1,7 @@
 package error
 
 import (
-	"fmt"
+	"errors"
 	"testing"
 )
 
@@ -13,18 +13,18 @@ func TestAsOlmErr(t *testing.T) {
 	}{
 		{
 			name:     "Install conflict error (match)",
-			err:      fmt.Errorf("Unable to continue with install: Deployment \"my-deploy\" in namespace \"my-namespace\" exists and cannot be imported into the current release"),
-			expected: InstallConflictErr(fmt.Errorf("Deployment 'my-deploy' already exists in namespace 'my-namespace'")),
+			err:      errors.New("Unable to continue with install: Deployment \"my-deploy\" in namespace \"my-namespace\" exists and cannot be imported into the current release"),
+			expected: errors.New("Deployment 'my-deploy' already exists in namespace 'my-namespace'"),
 		},
 		{
 			name:     "Install conflict error (no match)",
-			err:      fmt.Errorf("Unable to continue with install: because of something"),
-			expected: InstallConflictErr(fmt.Errorf("A conflict prevented the installation from proceeding. Please ensure there are no conflicting resources already present in the cluster.")),
+			err:      errors.New("Unable to continue with install: because of something"),
+			expected: errors.New("An unexpected error prevented the operator from succeeding"),
 		},
 		{
 			name:     "Unknown error",
-			err:      fmt.Errorf("some unknown error"),
-			expected: UnknownErr(fmt.Errorf("some unknown error")),
+			err:      errors.New("some unknown error"),
+			expected: errors.New("An unexpected error prevented the operator from succeeding"),
 		},
 		{
 			name:     "Nil error",

internal/action/helm.go

@@ -1,22 +1,80 @@
 package action
 
 import (
+	"context"
+
 	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/release"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	actionclient "github.com/operator-framework/helm-operator-plugins/pkg/client"
 
-	"github.com/operator-framework/helm-operator-plugins/pkg/client"
+	olmv1error "github.com/operator-framework/operator-controller/internal/action/error"
 )
 
+type ActionClientGetter struct {
+	actionclient.ActionClientGetter
+}
+
+func (a ActionClientGetter) ActionClientFor(ctx context.Context, obj client.Object) (actionclient.ActionInterface, error) {
+	ac, err := a.ActionClientGetter.ActionClientFor(ctx, obj)
+	if err != nil {
+		return nil, err
+	}
+	return ActionClient{
+		ActionInterface:             ac,
+		actionClientErrorTranslator: olmv1error.AsOlmErr,
+	}, nil
+}
+
+func NewWrappedActionClientGetter(acg actionclient.ActionConfigGetter, opts ...actionclient.ActionClientGetterOption) (*ActionClientGetter, error) {
+	ag, err := actionclient.NewActionClientGetter(acg, opts...)
+	if err != nil {
+		return nil, err
+	}
+	return &ActionClientGetter{
+		ActionClientGetter: ag,
+	}, nil
+}
+
+type ActionClientErrorTranslator func(err error) error
+
 type ActionClient struct {
-	client.ActionInterface
+	actionclient.ActionInterface
+	actionClientErrorTranslator ActionClientErrorTranslator
 }
 
-func (a ActionClient) Install(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...client.InstallOption) (*release.Release, error) {
+func NewWrappedActionClient(ca actionclient.ActionInterface, errTranslator ActionClientErrorTranslator) *ActionClient {
+	return &ActionClient{
+		ActionInterface:             ca,
+		actionClientErrorTranslator: errTranslator,
+	}
+}
+
+func (a ActionClient) Install(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...actionclient.InstallOption) (*release.Release, error) {
 	rel, err := a.ActionInterface.Install(name, namespace, chrt, vals, opts...)
+	err = a.actionClientErrorTranslator(err)
+	return rel, err
+}
+
+func (a ActionClient) Upgrade(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...actionclient.UpgradeOption) (*release.Release, error) {
+	rel, err := a.ActionInterface.Upgrade(name, namespace, chrt, vals, opts...)
+	err = a.actionClientErrorTranslator(err)
 	return rel, err
 }
 
-func (a ActionClient) Upgrade(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...client.UpgradeOption) (*release.Release, error) {
-	re, err := a.ActionInterface.Upgrade(name, namespace, chrt, vals, opts...)
-	return re, err
+func (a ActionClient) Uninstall(name string, opts ...actionclient.UninstallOption) (*release.UninstallReleaseResponse, error) {
+	resp, err := a.ActionInterface.Uninstall(name, opts...)
+	err = a.actionClientErrorTranslator(err)
+	return resp, err
+}
+
+func (a ActionClient) Get(name string, opts ...actionclient.GetOption) (*release.Release, error) {
+	resp, err := a.ActionInterface.Get(name, opts...)
+	err = a.actionClientErrorTranslator(err)
+	return resp, err
+}
+
+func (a ActionClient) Reconcile(rel *release.Release) error {
+	return a.actionClientErrorTranslator(a.ActionInterface.Reconcile(rel))
 }

internal/action/helm_test.go

@@ -0,0 +1,71 @@
+package action
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"helm.sh/helm/v3/pkg/chart"
+	"helm.sh/helm/v3/pkg/release"
+
+	"github.com/operator-framework/helm-operator-plugins/pkg/client"
+)
+
+var _ client.ActionInterface = &fakeActionClient{}
+
+type fakeActionClient struct {
+	err error
+}
+
+func (f fakeActionClient) Get(name string, opts ...client.GetOption) (*release.Release, error) {
+	return nil, f.err
+}
+
+func (f fakeActionClient) Install(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...client.InstallOption) (*release.Release, error) {
+	return nil, f.err
+}
+
+func (f fakeActionClient) Upgrade(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...client.UpgradeOption) (*release.Release, error) {
+	return nil, f.err
+}
+
+func (f fakeActionClient) Uninstall(name string, opts ...client.UninstallOption) (*release.UninstallReleaseResponse, error) {
+	return nil, f.err
+}
+
+func (f fakeActionClient) Reconcile(rel *release.Release) error {
+	return f.err
+}
+
+func TestActionClientErrorTranslation(t *testing.T) {
+	expectedErr := fmt.Errorf("something other error")
+	errTranslator := func(originalErr error) error {
+		return expectedErr
+	}
+
+	ac := fakeActionClient{
+		err: fmt.Errorf("some error"),
+	}
+
+	wrappedAc := NewWrappedActionClient(ac, errTranslator)
+
+	// Get
+	_, err := wrappedAc.Get("something")
+	assert.Equal(t, expectedErr, err, "expected Get() to return translated error")
+
+	// Install
+	_, err = wrappedAc.Install("something", "somethingelse", nil, nil)
+	assert.Equal(t, expectedErr, err, "expected Install() to return translated error")
+
+	// Uninstall
+	_, err = wrappedAc.Uninstall("something")
+	assert.Equal(t, expectedErr, err, "expected Uninstall() to return translated error")
+
+	// Upgrade
+	_, err = wrappedAc.Upgrade("something", "somethingelse", nil, nil)
+	assert.Equal(t, expectedErr, err, "expected Upgrade() to return translated error")
+
+	// Reconcile
+	err = wrappedAc.Reconcile(nil)
+	assert.Equal(t, expectedErr, err, "expected Reconcile() to return translated error")
+}