From e6f66fb77a728738910657c3b8544475182cc758 Mon Sep 17 00:00:00 2001
From: John Niang <johnniang@fastmail.com>
Date: Fri, 29 Oct 2021 09:59:55 +0800
Subject: [PATCH] Fix wrong check in function getCompletionTimeFromObject
 (#237)

* Fix wrong check in function getCompletionTimeFromObject

Signed-off-by: John Niang <johnniang@fastmail.com>

* Make sure item without completion time be at the end of items
---
 kubectl-plugin/pipeline/gc.go      | 12 +++-
 kubectl-plugin/pipeline/gc_test.go | 92 +++++++++++++++++++++++++++++-
 2 files changed, 100 insertions(+), 4 deletions(-)

diff --git a/kubectl-plugin/pipeline/gc.go b/kubectl-plugin/pipeline/gc.go
index 43d9075..b5bf38f 100644
--- a/kubectl-plugin/pipeline/gc.go
+++ b/kubectl-plugin/pipeline/gc.go
@@ -2,7 +2,11 @@ package pipeline
 
 import (
 	"context"
+	"errors"
 	"fmt"
+	"sort"
+	"time"
+
 	"github.com/kubesphere-sigs/ks/kubectl-plugin/common"
 	"github.com/kubesphere-sigs/ks/kubectl-plugin/pipeline/option"
 	"github.com/kubesphere-sigs/ks/kubectl-plugin/types"
@@ -10,8 +14,6 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/client-go/dynamic"
-	"sort"
-	"time"
 )
 
 func newGCCmd(client dynamic.Interface) (cmd *cobra.Command) {
@@ -116,7 +118,8 @@ func ascOrderWithCompletionTime(items []unstructured.Unstructured) {
 			return false
 		}
 		if rightCompletionTime, err = getCompletionTimeFromObject(right.Object); err != nil {
-			return false
+			// make sure that item without completion time be at the end of items
+			return true
 		}
 
 		return leftCompletionTime.Before(rightCompletionTime)
@@ -131,6 +134,9 @@ func getCompletionTimeFromObject(obj map[string]interface{}) (completionTime tim
 	if completionTimeStr, ok, err = unstructured.NestedString(obj, "status", "completionTime"); ok && err == nil {
 		completionTime, err = time.Parse(time.RFC3339, completionTimeStr)
 	}
+	if !ok {
+		err = errors.New("no status.completionTime field found")
+	}
 	return
 }
 
diff --git a/kubectl-plugin/pipeline/gc_test.go b/kubectl-plugin/pipeline/gc_test.go
index b73cab6..f01f0a5 100644
--- a/kubectl-plugin/pipeline/gc_test.go
+++ b/kubectl-plugin/pipeline/gc_test.go
@@ -1,19 +1,30 @@
 package pipeline
 
 import (
+	"reflect"
+	"testing"
+	"time"
+
 	"github.com/stretchr/testify/assert"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"testing"
 )
 
 func TestDescOrderWithCompletionTime(t *testing.T) {
 	items := []unstructured.Unstructured{{
+		Object: map[string]interface{}{
+			"flag": "3",
+		},
+	}, {
 		Object: map[string]interface{}{
 			"status": map[string]interface{}{
 				"completionTime": "2021-09-28T01:39:13Z",
 			},
 			"flag": "0",
 		},
+	}, {
+		Object: map[string]interface{}{
+			"flag": "4",
+		},
 	}, {
 		Object: map[string]interface{}{
 			"status": map[string]interface{}{
@@ -33,4 +44,83 @@ func TestDescOrderWithCompletionTime(t *testing.T) {
 	ascOrderWithCompletionTime(items)
 	flag, _, _ := unstructured.NestedString(items[0].Object, "flag")
 	assert.Equal(t, "2", flag)
+	// make sure that object without status.completionTime be at the end of the result
+	flag, _, _ = unstructured.NestedString(items[3].Object, "flag")
+	assert.Equal(t, "3", flag)
+	flag, _, _ = unstructured.NestedString(items[4].Object, "flag")
+	assert.Equal(t, "4", flag)
+}
+
+func Test_getCompletionTimeFromObject(t *testing.T) {
+	completionTime := time.Now()
+	completionTimeStr := completionTime.Format(time.RFC3339)
+	expectedCompletionTime, _ := time.Parse(time.RFC3339, completionTimeStr)
+	type args struct {
+		obj map[string]interface{}
+	}
+	tests := []struct {
+		name               string
+		args               args
+		wantCompletionTime time.Time
+		wantErr            bool
+	}{{
+		name: "Nil object",
+		args: args{
+			obj: nil,
+		},
+		wantErr: true,
+	}, {
+		name: "Without completionTime field",
+		args: args{
+			obj: map[string]interface{}{
+				"status": map[string]interface{}{
+					"updateTime": "",
+				},
+			},
+		},
+		wantErr: true,
+	}, {
+		name: "With an empty completionTime",
+		args: args{
+			obj: map[string]interface{}{
+				"status": map[string]interface{}{
+					"completionTime": "",
+				},
+			},
+		},
+		wantErr: true,
+	}, {
+		name: "Has completion time with RFC3339 layout",
+		args: args{
+			obj: map[string]interface{}{
+				"status": map[string]interface{}{
+					"completionTime": completionTime.Format(time.RFC3339),
+				},
+			},
+		},
+		wantCompletionTime: expectedCompletionTime,
+	}, {
+		name: "Has a invalid completion time",
+		args: args{
+			obj: map[string]interface{}{
+				"status": map[string]interface{}{
+					"completionTime": completionTime.Format(time.RFC1123),
+				},
+			},
+		},
+		wantErr: true,
+	},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gotCompletionTime, err := getCompletionTimeFromObject(tt.args.obj)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("getCompletionTimeFromObject() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(gotCompletionTime, tt.wantCompletionTime) {
+				t.Errorf("getCompletionTimeFromObject() = %v, want %v", gotCompletionTime, tt.wantCompletionTime)
+			}
+		})
+	}
 }