[GitHub] [yunikorn-k8shim] wilfred-s commented on a diff in pull request #462: [YUNIKORN-1314] The current UserkeyLabel is showed in log when user name couldn't be found in pod's labels.

[GitBox <gi...@apache.org>]

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


##########
pkg/common/utils/utils.go:
##########
@@ -240,18 +240,21 @@ func MergeMaps(first, second map[string]string) map[string]string {
 // find user name from pod label
 func GetUserFromPod(pod *v1.Pod) string {
 	userLabelKey := conf.GetSchedulerConf().UserLabelKey
-	// User name to be defined in labels
-	for name, value := range pod.Labels {
-		if name == userLabelKey {
-			log.Logger().Info("Found user name from pod labels.",
-				zap.String("userLabel", userLabelKey), zap.String("user", value))
-			return value
-		}
+	// UserLabelKey should not be empty
+	if len(userLabelKey) == 0 {
+		userLabelKey = constants.DefaultUserLabel
+		log.Logger().Debug("userLabelKey is empty and replaced by default setting",
+			zap.String("userLabel", userLabelKey))

Review Comment:
   The default is documented and we should not need to print it every time we try to retrieve a user, simple like this: "userLabelKey not set using default value"
   
   Need to also handle this as part of a unit test, codecov complains now



##########
pkg/common/utils/utils.go:
##########
@@ -240,18 +240,21 @@ func MergeMaps(first, second map[string]string) map[string]string {
 // find user name from pod label
 func GetUserFromPod(pod *v1.Pod) string {
 	userLabelKey := conf.GetSchedulerConf().UserLabelKey
-	// User name to be defined in labels
-	for name, value := range pod.Labels {
-		if name == userLabelKey {
-			log.Logger().Info("Found user name from pod labels.",
-				zap.String("userLabel", userLabelKey), zap.String("user", value))
-			return value
-		}
+	// UserLabelKey should not be empty
+	if len(userLabelKey) == 0 {
+		userLabelKey = constants.DefaultUserLabel
+		log.Logger().Debug("userLabelKey is empty and replaced by default setting",
+			zap.String("userLabel", userLabelKey))
 	}
-	value := constants.DefaultUser
 
+	// User name to be defined in labels
+	if username, ok := pod.Labels[userLabelKey]; ok {

Review Comment:
   Can the label be found and still return an empty username?
   We need to handle that case and still set the default user



-- 
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