Skip to content

Add batch error handling to Notifications page#3933

Merged
jpellizzari merged 2 commits intomainfrom
3634-notif-errors
Aug 10, 2023
Merged

Add batch error handling to Notifications page#3933
jpellizzari merged 2 commits intomainfrom
3634-notif-errors

Conversation

@jpellizzari
Copy link
Copy Markdown
Contributor

@jpellizzari jpellizzari commented Aug 9, 2023

Closes #3634

Adds an ErrorList component to Core, heavily inspired by the one used in EE. This also exports the ErrorList component so that it may be imported in EE and eventually subsume the AlertListErrors component (there is nothing wrong with that component currently, but this will enforce Consistency:tm:).

There are now two error "classes" on the page:

Error when we can't reach the backend:
Screenshot from 2023-08-09 12-40-04

Batched errors from various clusters (backend returns lots of errors):
Screenshot from 2023-08-09 12-40-22

Simulated the error state using this:

 core/server/objects.go | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/core/server/objects.go b/core/server/objects.go
index 0bb6e26d5..f99290bbc 100644
--- a/core/server/objects.go
+++ b/core/server/objects.go
@@ -14,6 +14,7 @@ import (
 	pb "github.com/weaveworks/weave-gitops/pkg/api/core"
 	"github.com/weaveworks/weave-gitops/pkg/run/constants"
 	"github.com/weaveworks/weave-gitops/pkg/server/auth"
+	"github.com/weaveworks/weave-gitops/pkg/utils"
 	v1 "k8s.io/api/apps/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -148,6 +149,15 @@ func (cs *coreServer) ListObjects(ctx context.Context, msg *pb.ListObjectsReques
 		}
 	}
 
+	if msg.Kind == "Provider" {
+
+		for i := 0; i < 50; i++ {
+			msg, _ := utils.GenerateRandomString(5, 10)
+			respErrors = append(respErrors, &pb.ListError{ClusterName: "provider", Message: fmt.Sprintf("such and such is forbidden in namespace: %v", msg)})
+		}
+
+	}
+
 	return &pb.ListObjectsResponse{
 		Objects: results,
 		Errors:  respErrors,

@jpellizzari
Copy link
Copy Markdown
Contributor Author

Authoring the PR made me realize the Alert icons are inconsistent, so I fixed that in a second commit:
Screenshot from 2023-08-09 12-54-49

Copy link
Copy Markdown
Contributor

@joshri joshri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - might be worth distinguishing between the request errors and permissions errors somehow...maybe permissions errors are more like warnings? Just a thought for the future

@jpellizzari
Copy link
Copy Markdown
Contributor Author

LGTM - might be worth distinguishing between the request errors and permissions errors somehow...maybe permissions errors are more like warnings? Just a thought for the future

I think "errors is errors" as far as the backend is concerned. It doesn't differentiate between permissions and request errors.

Before this change, if you got lots of errors on the Notification page, they would all stack up and take the entire page height. After, they will be organized in a navigable list, like how it done in Enterprise.
@jpellizzari jpellizzari merged commit f642468 into main Aug 10, 2023
@jpellizzari jpellizzari deleted the 3634-notif-errors branch August 10, 2023 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Notifications page cluttered with error messages

2 participants