Skip to content

Commit

Permalink
Return user friendly error when package doesn't exist
Browse files Browse the repository at this point in the history
Signed-off-by: rcmadhankumar <[email protected]>
  • Loading branch information
rcmadhankumar committed Jan 9, 2024
1 parent 1185416 commit b29b1d2
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 9 deletions.
8 changes: 4 additions & 4 deletions pkg/apiserver/registry/datapackaging/package_crd_rest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/internalpackaging/v1alpha1"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apiserver/apis/datapackaging"
datapkgreg "github.com/vmware-tanzu/carvel-kapp-controller/pkg/apiserver/registry/datapackaging"
Expand Down Expand Up @@ -146,6 +147,7 @@ func TestPackageVersionGetPresentInOnlyNS(t *testing.T) {
func TestPackageVersionGetNotFound(t *testing.T) {
namespacedPackageVersion := excludedNonGlobalIntPackageVersion()
name := namespacedPackageName
expectedError := "package.data.packaging.carvel.dev \"" + name + "\" not found"

internalClient := fake.NewSimpleClientset(namespacedPackageVersion)
fakeCoreClient := k8sfake.NewSimpleClientset(namespace())
Expand All @@ -157,10 +159,8 @@ func TestPackageVersionGetNotFound(t *testing.T) {
t.Fatalf("Expected get operation to fail, but it didn't")
}

if !errors.IsNotFound(err) {
t.Fatalf("Expected a not found error, got: %v", err)
}

assert.True(t, errors.IsNotFound(err))
assert.ErrorContains(t, err, expectedError)
}

func TestPackageVersionGetPreferNS(t *testing.T) {
Expand Down
34 changes: 29 additions & 5 deletions pkg/apiserver/registry/datapackaging/package_storage_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package datapackaging
import (
"context"
"encoding/base32"
"errors"
"fmt"
"strings"

Expand All @@ -14,7 +15,7 @@ import (
datapkgingv1alpha1 "github.com/vmware-tanzu/carvel-kapp-controller/pkg/apiserver/apis/datapackaging/v1alpha1"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apiserver/watchers"
internalclient "github.com/vmware-tanzu/carvel-kapp-controller/pkg/client/clientset/versioned"
"k8s.io/apimachinery/pkg/api/errors"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/watch"
Expand Down Expand Up @@ -65,7 +66,7 @@ func (t PackageTranslator) ToExternalObj(intObj *internalpkgingv1alpha1.Internal
var err error
obj.Name, err = t.ToExternalName(intObj.Name)
if err != nil {
return nil, errors.NewInternalError(fmt.Errorf("decoding internal obj name '%s': %v", intObj.Name, err))
return nil, apierrors.NewInternalError(fmt.Errorf("decoding internal obj name '%s': %v", intObj.Name, err))
}

// Self link is deprecated and planned for removal, so we don't translate it
Expand Down Expand Up @@ -122,10 +123,10 @@ func (t PackageTranslator) ToExternalWatcher(intObjWatcher watch.Interface, fiel
evt.Object, err = t.ToExternalObj(intpkg)
if err != nil {
var status metav1.Status
if statusErr, ok := err.(*errors.StatusError); ok {
if statusErr, ok := err.(*apierrors.StatusError); ok {
status = statusErr.Status()
} else {
status = errors.NewInternalError(err).Status()
status = apierrors.NewInternalError(err).Status()
}
return watch.Event{Type: watch.Error, Object: &status}
}
Expand All @@ -152,7 +153,30 @@ func (t PackageTranslator) ToExternalWatcher(intObjWatcher watch.Interface, fiel
}

func (t PackageTranslator) ToExternalError(err error) error {
// TODO: implement

status, ok := err.(apierrors.APIStatus)
if !(ok || errors.As(err, &status)) {
return err
}

details := status.Status().Details
if details != nil && details.Kind == "internalpackages" && details.Group == internalpkgingv1alpha1.SchemeGroupVersion.Group {
packageName, translateErr := t.ToExternalName(details.Name)
if translateErr != nil {
return err
}

switch status.Status().Reason {
case metav1.StatusReasonNotFound:
return apierrors.NewNotFound(datapkgingv1alpha1.Resource("package"), packageName)
case metav1.StatusReasonAlreadyExists:
return apierrors.NewAlreadyExists(datapkgingv1alpha1.Resource("package"), packageName)
// TODO Handle other types of errors
default:
return err
}
}

return err
}

Expand Down
21 changes: 21 additions & 0 deletions test/e2e/kappcontroller/package_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apiserver/apis/datapackaging/v1alpha1"
"github.com/vmware-tanzu/carvel-kapp-controller/test/e2e"
"sigs.k8s.io/yaml"
Expand Down Expand Up @@ -409,3 +410,23 @@ spec:
}
})
}

func TestPackageNotFound(t *testing.T) {
env := e2e.BuildEnv(t)
logger := e2e.Logger{}
k := e2e.Kubectl{t, env.Namespace, logger}
packageName := "foo.1.0.0"
expectedError := "stderr: 'Error from server (NotFound): package.data.packaging.carvel.dev \"foo.1.0.0\" not found"

logger.Section("Get Package", func() {
_, err := k.RunWithOpts([]string{"get", "package", packageName}, e2e.RunOpts{AllowError: true})
assert.NotNil(t, err)
assert.ErrorContains(t, err, expectedError)
})

logger.Section("delete Package", func() {
_, err := k.RunWithOpts([]string{"delete", "package", packageName}, e2e.RunOpts{AllowError: true})
assert.NotNil(t, err)
assert.ErrorContains(t, err, expectedError)
})
}

0 comments on commit b29b1d2

Please sign in to comment.