[GitHub] [yunikorn-k8shim] wilfred-s commented on a diff in pull request #594: [YUNIKORN-1273] Add configurable option to have unique application ids in a namespace

["wilfred-s (via GitHub)" <gi...@apache.org>]

wilfred-s commented on code in PR #594:
URL: https://github.com/apache/yunikorn-k8shim/pull/594#discussion_r1198556303


##########
pkg/admission/util.go:
##########
@@ -75,3 +76,18 @@ func convert2Namespace(obj interface{}) *v1.Namespace {
 	log.Logger().Warn("cannot convert to *v1.Namespace", zap.Stringer("type", reflect.TypeOf(obj)))
 	return nil
 }
+
+// generate appID based on the namespace value,
+// and the max length of the ID is 63 chars.

Review Comment:
   nit: update comment to align with implementation



##########
pkg/admission/util_test.go:
##########
@@ -38,28 +39,84 @@ func createConfigWithOverrides(overrides map[string]string) *conf.AdmissionContr
 	return conf.NewAdmissionControllerConf([]*v1.ConfigMap{nil, {Data: overrides}})
 }
 
-// nolint: funlen
-func TestUpdatePodLabelForAdmissionController(t *testing.T) {
-	// verify when appId/queue are not given,
-	pod := &v1.Pod{
+func createMinimalTestingPod() *v1.Pod {
+	return &v1.Pod{
 		TypeMeta: metav1.TypeMeta{
 			Kind:       "Pod",
 			APIVersion: "v1",
 		},
-		ObjectMeta: metav1.ObjectMeta{
+		ObjectMeta: metav1.ObjectMeta{},
+		Spec:       v1.PodSpec{},
+		Status:     v1.PodStatus{},
+	}
+}
+
+func createTestingPodWithMeta() *v1.Pod {
+	pod := createMinimalTestingPod()
+
+	pod.ObjectMeta =
+		metav1.ObjectMeta{
 			Name:            "a-test-pod",
 			Namespace:       "default",
 			UID:             "7f5fd6c5d5",
 			ResourceVersion: "10654",
 			Labels: map[string]string{
 				"random": "random",
 			},
-		},
-		Spec:   v1.PodSpec{},
-		Status: v1.PodStatus{},
-	}
+		}
 
-	if result := updatePodLabel(pod, "default"); result != nil {
+	return pod
+}
+
+func createTestingPodWithCustomFields(ns string) *v1.Pod {
+	pod := createMinimalTestingPod()
+	pod.ObjectMeta =
+		metav1.ObjectMeta{
+			Namespace: ns,
+			UID:       "abcd1234-5678-efgh-90ij-klmnopqrstuv",
+		}
+
+	return pod
+}
+
+func createTestingPodWithAppId() *v1.Pod {
+	pod := createTestingPodWithMeta()
+	pod.ObjectMeta.Labels["applicationId"] = "app-0001"
+
+	return pod
+}
+
+func createTestingPodWithGenerateName() *v1.Pod {
+	pod := createMinimalTestingPod()
+	pod.ObjectMeta.GenerateName = "some-pod-"
+
+	return pod
+}
+
+func createTestingPodWithQueue() *v1.Pod {
+	pod := createTestingPodWithMeta()
+	pod.ObjectMeta.Labels["queue"] = "root.abc"
+
+	return pod
+}
+
+func createTestingPodNoNamespaceAndLabels() *v1.Pod {
+	pod := createMinimalTestingPod()
+	pod.ObjectMeta =
+		metav1.ObjectMeta{
+			Name:            "a-test-pod",
+			UID:             "7f5fd6c5d5",
+			ResourceVersion: "10654",
+		}
+	return pod
+}
+
+// nolint: funlen

Review Comment:
   we should not need this anymore function is now only 65 lines



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org