Merge pull request #48659 from shiywang/fix-sort

Automatic merge from submit-queue

Fix sort-by output problem

Fixes https://github.com/kubernetes/kubectl/issues/43

This bug was original introduced in pr here: https://github.com/kubernetes/kubernetes/pull/46265, I think next time if we touch something printer related package, maybe should let @smarterclayton have a review, although he is pretty busy I guess : ) and that package also changed a lot recently since he's been working on refactoring.
 
this is a quick and dirty fix, not sure if there's better way, I will add some regression test soon...

@kubernetes/sig-cli-pr-reviews 

```release-note
NONE
```

/assign @mengqiy 
/assign @smarterclayton

hack/make-rules/test-cmd-util.sh

@@ -3921,6 +3921,26 @@ run_kubectl_sort_by_tests() {
   kubectl get pods --sort-by="{metadata.name}"
   kubectl get pods --sort-by="{metadata.creationTimestamp}"
 
+  ### sort-by should works if pod exists
+  # Create POD
+  # Pre-condition: no POD exists
+  kube::test::get_object_assert pods "{{range.items}}{{$id_field}}:{{end}}" ''
+  # Command
+  kubectl create "${kube_flags[@]}" -f test/fixtures/doc-yaml/admin/limitrange/valid-pod.yaml
+  # Post-condition: valid-pod is created
+  kube::test::get_object_assert pods "{{range.items}}{{$id_field}}:{{end}}" 'valid-pod:'
+  # Check output of sort-by
+  output_message=$(kubectl get pods --sort-by="{metadata.name}")
+  kube::test::if_has_string "${output_message}" "valid-pod"
+  ### Clean up
+  # Pre-condition: valid-pod exists
+  kube::test::get_object_assert pods "{{range.items}}{{$id_field}}:{{end}}" 'valid-pod:'
+  # Command
+  kubectl delete "${kube_flags[@]}" pod valid-pod --grace-period=0 --force
+  # Post-condition: valid-pod doesn't exist
+  kube::test::get_object_assert pods "{{range.items}}{{$id_field}}:{{end}}" ''
+
+
   set +o nounset
   set +o errexit
 }

pkg/kubectl/cmd/util/factory_builder.go

@@ -32,7 +32,6 @@ import (
 	"k8s.io/kubernetes/pkg/kubectl/plugins"
 	"k8s.io/kubernetes/pkg/kubectl/resource"
 	"k8s.io/kubernetes/pkg/printers"
-	printersinternal "k8s.io/kubernetes/pkg/printers/internalversion"
 )
 
 type ring2Factory struct {
@@ -103,16 +102,6 @@ func (f *ring2Factory) PrinterForMapping(cmd *cobra.Command, isLocal bool, outpu
 
 		printer = printers.NewVersionedPrinter(printer, mapping.ObjectConvertor, version, mapping.GroupVersionKind.GroupVersion())
 
-	} else {
-		// We add handlers to the printer in case it is printers.HumanReadablePrinter.
-		// printers.AddHandlers expects concrete type of printers.HumanReadablePrinter
-		// as its parameter because of this we have to do a type check on printer and
-		// extract out concrete HumanReadablePrinter from it. We are then able to attach
-		// handlers on it.
-		if humanReadablePrinter, ok := printer.(*printers.HumanReadablePrinter); ok {
-			printersinternal.AddHandlers(humanReadablePrinter)
-			printer = humanReadablePrinter
-		}
 	}
 
 	return printer, nil

pkg/kubectl/cmd/util/printing.go

@@ -26,6 +26,7 @@ import (
 	"k8s.io/kubernetes/pkg/kubectl"
 	"k8s.io/kubernetes/pkg/kubectl/resource"
 	"k8s.io/kubernetes/pkg/printers"
+	printersinternal "k8s.io/kubernetes/pkg/printers/internalversion"
 
 	"github.com/spf13/cobra"
 )
@@ -126,6 +127,13 @@ func PrinterForCommand(cmd *cobra.Command, outputOpts *printers.OutputOptions, m
 		return nil, err
 	}
 
+	// we try to convert to HumanReadablePrinter, if return ok, it must be no generic
+	// we execute AddHandlers() here before maybeWrapSortingPrinter so that we don't
+	// need to convert to delegatePrinter again then invoke AddHandlers()
+	if humanReadablePrinter, ok := printer.(*printers.HumanReadablePrinter); ok {
+		printersinternal.AddHandlers(humanReadablePrinter)
+	}
+
 	return maybeWrapSortingPrinter(cmd, printer), nil
 }