chore: improve application readability

Signed-off-by: yyzxw <1020938856@qq.com>

pkg/apis/application/v1alpha1/types.go

@@ -2627,6 +2627,17 @@ func (app *Application) IsFinalizerPresent(finalizer string) bool {
 	return getFinalizerIndex(app.ObjectMeta, finalizer) > -1
 }
 
+// Equals compares two instances of Application and returns true if instances spec is equal.
+func (app *Application) Equals(compare *Application) bool {
+	if app == nil || compare == nil {
+		return false
+	}
+	return reflect.DeepEqual(app.Spec, compare.Spec) &&
+		reflect.DeepEqual(app.Labels, compare.Labels) &&
+		reflect.DeepEqual(app.Annotations, compare.Annotations) &&
+		reflect.DeepEqual(app.Finalizers, compare.Finalizers)
+}
+
 // SetConditions updates the application status conditions for a subset of evaluated types.
 // If the application has a pre-existing condition of a type that is not in the evaluated list,
 // it will be preserved. If the application has a pre-existing condition of a type that

pkg/apis/application/v1alpha1/types_test.go

@@ -646,7 +646,7 @@ func TestAppProject_ValidateDestinations(t *testing.T) {
 	err = p.ValidateProject()
 	assert.NoError(t, err)
 
-	//no duplicates allowed
+	// no duplicates allowed
 	p.Spec.Destinations = []ApplicationDestination{validDestination, validDestination}
 	err = p.ValidateProject()
 	assert.Error(t, err)
@@ -3610,3 +3610,77 @@ func TestOptionalMapEquality(t *testing.T) {
 		})
 	}
 }
+
+func TestApplication_Equals(t *testing.T) {
+	tests := []struct {
+		name    string
+		args    *Application
+		compare *Application
+		want    bool
+	}{
+		{
+			name: "test compare with nil object",
+			args: &Application{},
+			want: false,
+		},
+		{
+			name: "test label not equal",
+			args: &Application{
+				Spec: newTestApp().Spec,
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						"foo": "bar",
+					},
+				},
+			},
+			compare: &Application{
+				Spec: newTestApp().Spec,
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{},
+				},
+			},
+			want: false,
+		},
+		{
+			name: "test spec not equal",
+			args: &Application{
+				Spec: ApplicationSpec{},
+			},
+			compare: &Application{
+				Spec: newTestApp().Spec,
+			},
+			want: false,
+		},
+		{
+			name: "test all equal",
+			args: &Application{
+				Spec: newTestApp().Spec,
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						"foo": "bar",
+					},
+					Annotations: map[string]string{
+						"foo": "bar",
+					},
+				},
+			},
+			compare: &Application{
+				Spec: newTestApp().Spec,
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						"foo": "bar",
+					},
+					Annotations: map[string]string{
+						"foo": "bar",
+					},
+				},
+			},
+			want: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equalf(t, tt.want, tt.args.Equals(tt.compare), "EqualSpec(%v)", tt.compare)
+		})
+	}
+}

server/application/application.go

@@ -6,7 +6,6 @@ import (
 	"errors"
 	"fmt"
 	"math"
-	"reflect"
 	"sort"
 	"strconv"
 	"strings"
@@ -297,31 +296,31 @@ func (s *Server) Create(ctx context.Context, q *application.ApplicationCreateReq
 	if q.GetApplication() == nil {
 		return nil, fmt.Errorf("error creating application: application is nil in request")
 	}
-	a := q.GetApplication()
+	app := q.GetApplication()
 
-	if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceApplications, rbacpolicy.ActionCreate, a.RBACName(s.ns)); err != nil {
+	if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceApplications, rbacpolicy.ActionCreate, app.RBACName(s.ns)); err != nil {
 		return nil, err
 	}
 
-	s.projectLock.RLock(a.Spec.GetProject())
-	defer s.projectLock.RUnlock(a.Spec.GetProject())
+	s.projectLock.RLock(app.Spec.GetProject())
+	defer s.projectLock.RUnlock(app.Spec.GetProject())
 
 	validate := true
 	if q.Validate != nil {
 		validate = *q.Validate
 	}
-	err := s.validateAndNormalizeApp(ctx, a, validate)
+	err := s.validateAndNormalizeApp(ctx, app, validate)
 	if err != nil {
 		return nil, fmt.Errorf("error while validating and normalizing app: %w", err)
 	}
 
-	appNs := s.appNamespaceOrDefault(a.Namespace)
+	appNs := s.appNamespaceOrDefault(app.Namespace)
 
 	if !s.isNamespaceEnabled(appNs) {
 		return nil, security.NamespaceNotPermittedError(appNs)
 	}
 
-	created, err := s.appclientset.ArgoprojV1alpha1().Applications(appNs).Create(ctx, a, metav1.CreateOptions{})
+	created, err := s.appclientset.ArgoprojV1alpha1().Applications(appNs).Create(ctx, app, metav1.CreateOptions{})
 	if err == nil {
 		s.logAppEvent(created, ctx, argo.EventReasonResourceCreated, "created application")
 		s.waitSync(created)
@@ -332,25 +331,20 @@ func (s *Server) Create(ctx context.Context, q *application.ApplicationCreateReq
 	}
 
 	// act idempotent if existing spec matches new spec
-	existing, err := s.appLister.Applications(appNs).Get(a.Name)
+	existing, err := s.appLister.Applications(appNs).Get(app.Name)
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "unable to check existing application details (%s): %v", appNs, err)
 	}
-	equalSpecs := reflect.DeepEqual(existing.Spec, a.Spec) &&
-		reflect.DeepEqual(existing.Labels, a.Labels) &&
-		reflect.DeepEqual(existing.Annotations, a.Annotations) &&
-		reflect.DeepEqual(existing.Finalizers, a.Finalizers)
-
-	if equalSpecs {
+	if app.Equals(existing) {
 		return existing, nil
 	}
 	if q.Upsert == nil || !*q.Upsert {
 		return nil, status.Errorf(codes.InvalidArgument, "existing application spec is different, use upsert flag to force update")
 	}
-	if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceApplications, rbacpolicy.ActionUpdate, a.RBACName(s.ns)); err != nil {
+	if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceApplications, rbacpolicy.ActionUpdate, app.RBACName(s.ns)); err != nil {
 		return nil, err
 	}
-	updated, err := s.updateApp(existing, a, ctx, true)
+	updated, err := s.updateApp(existing, app, ctx, true)
 	if err != nil {
 		return nil, fmt.Errorf("error updating application: %w", err)
 	}
@@ -638,6 +632,8 @@ func (s *Server) Get(ctx context.Context, q *application.ApplicationQuery) (*app
 
 	project := ""
 	projects := getProjectsFromApplicationQuery(*q)
+
+	// FIXME: actually support zero or one project, why not use string?
 	if len(projects) == 1 {
 		project = projects[0]
 	} else if len(projects) > 1 {
@@ -663,7 +659,7 @@ func (s *Server) Get(ctx context.Context, q *application.ApplicationQuery) (*app
 	}
 	appIf := s.appclientset.ArgoprojV1alpha1().Applications(appNs)
 
-	// subscribe early with buffered channel to ensure we don't miss events
+	// subscribe early with a buffered channel to ensure we don't miss events
 	events := make(chan *appv1.ApplicationWatchEvent, watchAPIBufferSize)
 	unsubscribe := s.appBroadcaster.Subscribe(events, func(event *appv1.ApplicationWatchEvent) bool {
 		return event.Application.Name == appName && event.Application.Namespace == appNs
@@ -1274,7 +1270,7 @@ func (s *Server) GetResource(ctx context.Context, q *application.ApplicationReso
 		return nil, err
 	}
 
-	// make sure to use specified resource version if provided
+	// make sure to use a specified resource version if provided
 	if q.GetVersion() != "" {
 		res.Version = q.GetVersion()
 	}
@@ -1593,8 +1589,8 @@ func (s *Server) PodLogs(q *application.ApplicationPodLogsQuery, ws application.
 
 		streams = append(streams, logStream)
 		go func() {
-			// if k8s failed to start steaming logs (typically because Pod is not ready yet)
-			// then the error should be shown in the UI so that user know the reason
+			// if k8s failed to start steaming logs (typically because Pod is not ready yet),
+			// then the error should be shown in the UI so that the user knows the reason
 			if err != nil {
 				logStream <- logEntry{line: err.Error()}
 			} else {
@@ -1664,7 +1660,7 @@ func (s *Server) PodLogs(q *application.ApplicationPodLogsQuery, ws application.
 	}
 }
 
-// from all of the treeNodes, get the pod who meets the criteria or whose parents meets the criteria
+// from all the treeNodes, get the pod who meets the criteria or whose parents meets the criteria
 func getSelectedPods(treeNodes []appv1.ResourceNode, q *application.ApplicationPodLogsQuery) []appv1.ResourceNode {
 	var pods []appv1.ResourceNode
 	isTheOneMap := make(map[string]bool)
@@ -1678,7 +1674,7 @@ func getSelectedPods(treeNodes []appv1.ResourceNode, q *application.ApplicationP
 	return pods
 }
 
-// check is currentNode is matching with group, kind, and name, or if any of its parents matches
+// check is currentNode is matching with a group, kind, and name, or if any of its parents matches
 func isTheSelectedOne(currentNode *appv1.ResourceNode, q *application.ApplicationPodLogsQuery, resourceNodes []appv1.ResourceNode, isTheOneMap map[string]bool) bool {
 	exist, value := isTheOneMap[currentNode.UID]
 	if exist {
@@ -1699,8 +1695,8 @@ func isTheSelectedOne(currentNode *appv1.ResourceNode, q *application.Applicatio
 	}
 
 	for _, parentResource := range currentNode.ParentRefs {
-		//look up parentResource from resourceNodes
-		//then check if the parent isTheSelectedOne
+		// look up parentResource from resourceNodes
+		// then check if the parent isTheSelectedOne
 		for _, resourceNode := range resourceNodes {
 			if resourceNode.Namespace == parentResource.Namespace &&
 				resourceNode.Name == parentResource.Name &&
@@ -1854,7 +1850,7 @@ func (s *Server) Rollback(ctx context.Context, rollbackReq *application.Applicat
 		return nil, status.Errorf(codes.InvalidArgument, "application %s does not have deployment with id %v", a.QualifiedName(), rollbackReq.GetId())
 	}
 	if deploymentInfo.Source.IsZero() {
-		// Since source type was introduced to history starting with v0.12, and is now required for
+		// Since a source type was introduced to history starting with v0.12, and is now required for
 		// rollback, we cannot support rollback to revisions deployed using Argo CD v0.11 or below
 		return nil, status.Errorf(codes.FailedPrecondition, "cannot rollback to revision deployed with Argo CD v0.11 or lower. sync to revision instead.")
 	}

server/application/application_test.go

@@ -208,7 +208,7 @@ func newTestAppServerWithEnforcerConfigure(f func(*rbac.Enforcer), t *testing.T,
 	// populate the app informer with the fake objects
 	appInformer := factory.Argoproj().V1alpha1().Applications().Informer()
 	// TODO(jessesuen): probably should return cancel function so tests can stop background informer
-	//ctx, cancel := context.WithCancel(context.Background())
+	// ctx, cancel := context.WithCancel(context.Background())
 	go appInformer.Run(ctx.Done())
 	if !k8scache.WaitForCacheSync(ctx.Done(), appInformer.HasSynced) {
 		panic("Timed out waiting for caches to sync")
@@ -1135,7 +1135,7 @@ func testListAppsWithLabels(t *testing.T, appQuery application.ApplicationQuery,
 			label:          "!key2",
 			expectedResult: []string{"App2", "App3"}},
 	}
-	//test valid scenarios
+	// test valid scenarios
 	for _, validTest := range validTests {
 		t.Run(validTest.testName, func(t *testing.T) {
 			appQuery.Selector = &validTest.label
@@ -1161,7 +1161,7 @@ func testListAppsWithLabels(t *testing.T, appQuery application.ApplicationQuery,
 			label:       "key1<value1",
 			errorMesage: "error parsing the selector"},
 	}
-	//test invalid scenarios
+	// test invalid scenarios
 	for _, invalidTest := range invalidTests {
 		t.Run(invalidTest.testName, func(t *testing.T) {
 			appQuery.Selector = &invalidTest.label
@@ -2343,3 +2343,52 @@ func TestIsApplicationPermitted(t *testing.T) {
 		assert.True(t, permitted)
 	})
 }
+
+func Test_mergeStringMaps(t *testing.T) {
+	tests := []struct {
+		name string
+		args []map[string]string
+		want map[string]string
+	}{
+		{
+			name: "test empty maps",
+			args: []map[string]string{},
+			want: map[string]string{},
+		},
+		{
+			name: "test one map",
+			args: []map[string]string{{"a": "b"}},
+			want: map[string]string{"a": "b"},
+		},
+		{
+			name: "test mutil maps",
+			args: []map[string]string{
+				{
+					"key1": "val1",
+					"key2": "val2",
+				},
+				{
+					"key2": "val2",
+					"key3": "val3",
+				},
+				{
+					"key3": "val3",
+				},
+				{
+					"key4": "val4",
+				},
+			},
+			want: map[string]string{
+				"key1": "val1",
+				"key2": "val2",
+				"key3": "val3",
+				"key4": "val4",
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equalf(t, tt.want, mergeStringMaps(tt.args...), "mergeStringMaps(%v)", tt.args)
+		})
+	}
+}

server/application/logs.go

@@ -24,7 +24,7 @@ func parseLogsStream(podName string, stream io.ReadCloser, ch chan logEntry) {
 		line, err := bufReader.ReadString('\n')
 		if err == io.EOF {
 			eof = true
-			// stop if we reached end of stream and the next line is empty
+			// stop if we reached an end of stream and the next line is empty
 			if line == "" {
 				break
 			}
@@ -50,8 +50,8 @@ func parseLogsStream(podName string, stream io.ReadCloser, ch chan logEntry) {
 	}
 }
 
-// mergeLogStreams merge two stream of logs and ensures that merged logs are sorted by timestamp.
-// The implementation uses merge sort: method reads next log entry from each stream if one of streams is empty
+// mergeLogStreams merge two streams of logs and ensures that merged logs are sorted by timestamp.
+// The implementation uses merge sort: method reads next log entry from each stream if one of the streams is empty,
 // it waits for no longer than specified duration and then merges available entries.
 func mergeLogStreams(streams []chan logEntry, bufferingDuration time.Duration) chan logEntry {
 	merged := make(chan logEntry)
@@ -123,7 +123,7 @@ func mergeLogStreams(streams []chan logEntry, bufferingDuration time.Duration) c
 	go func() {
 		for range ticker.C {
 			sentAtLock.Lock()
-			// waited long enough for logs from each streams, send everything accumulated
+			// waited long enough for logs from each stream, send everything accumulated
 			if sentAt.Add(bufferingDuration).Before(time.Now()) {
 				_ = send(true)
 				sentAt = time.Now()

server/application/terminal.go

@@ -227,6 +227,7 @@ func (s *terminalHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, "Failed to start terminal session", http.StatusBadRequest)
 		return
 	}
+	defer session.Close()
 	defer session.Done()
 
 	// send pings across the WebSocket channel at regular intervals to keep it alive through
@@ -248,11 +249,8 @@ func (s *terminalHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 
 	if err != nil {
 		http.Error(w, "Failed to exec container", http.StatusBadRequest)
-		session.Close()
 		return
 	}
-
-	session.Close()
 }
 
 func podExists(treeNodes []appv1.ResourceNode, podName, namespace string) bool {