Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix kubectl sort-by metadata issue. #21694

Merged
merged 1 commit into from
Mar 7, 2016
Merged

Fix kubectl sort-by metadata issue. #21694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 22 additions & 2 deletions pkg/kubectl/cmd/get.go
View file
Open in desktop
Expand Up @@ -248,6 +248,23 @@ func RunGet(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []string
sorting, err := cmd.Flags().GetString("sort-by")
var sorter *kubectl.RuntimeSort
if err == nil && len(sorting) > 0 && len(objs) > 1 {
clientConfig, err := f.ClientConfig()
if err != nil {
return err
}

version, err := cmdutil.OutputVersion(cmd, clientConfig.GroupVersion)
if err != nil {
return err
}

for ix := range infos {
objs[ix], err = infos[ix].Mapping.ConvertToVersion(infos[ix].Object, version.String())
if err != nil {
return err
}
}

// TODO: questionable
if sorter, err = kubectl.SortObjects(f.Decoder(true), objs, sorting); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at first glance, rather than converting into versions objects, here, I think would expect to pass the output version to SortObjects... it is taking a fieldInput, which is not really meaningful unless paired with a group version. In SortObjects, can we decode to the specified groupversion? I'd like @smarterclayton's take on that as well...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect us to be sorting based on the JSON and the field path
qualifier only, or at minimum on the map[string]interface{}. It's a
riskier change though.

On Mon, Feb 22, 2016 at 1:37 PM, Jordan Liggitt notifications@github.com
wrote:

In pkg/kubectl/cmd/get.go
#21694 (comment)
:

  •   clientConfig, err := f.ClientConfig()

  •   if err != nil {

  •       return err

  •   }

  •   version, err := cmdutil.OutputVersion(cmd, clientConfig.GroupVersion)

  •   if err != nil {

  •       return err

  •   }

  •   objs, err = resource.AsVersionedObjects(infos, version.String(), f.JSONEncoder())

  •   if err != nil {

  •       return err

  •   }
    • // TODO: questionable
      if sorter, err = kubectl.SortObjects(f.Decoder(true), objs, sorting); err != nil {

at first glance, rather than converting into versions objects, here, I
think would expect to pass the output version to SortObjects... it is
taking a fieldInput, which is not really meaningful unless paired with a
group version. In SortObjects, can we decode to the specified
groupversion? I'd like @smarterclayton https://github.com/smarterclayton's
take on that as well...

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/21694/files#r53669745.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liggitt I am looking into what you suggested and i think it should be possible. The only issue with that I see is that once we decode objects into specified groupversion inside SortObjects, after sorting, any caller of sort-by would have converted versioned objects and it would require handling printing of those objects in the similar way as it is done currently in kubectl get. Because as I understand, it is not possible to print versioned objects directly but only internal types objects.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or another way might be to convert sorted and decoded obejcts back to their internal types and then return them. Although I am not sure how to handle this,

return err
Expand All @@ -262,10 +279,13 @@ func RunGet(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []string

for ix := range objs {
var mapping *meta.RESTMapping
var original runtime.Object
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still needed? Don't you want to print out a versioned object anyway?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to print versioned objects? Whenever I try to print versioned objects, for example v1, I get

error: unknown type &v1.Pod{TypeMeta:unversioned.TypeMeta{Kind:"Pod", APIVersion:"v1"},....

Complete error is here: http://fpaste.org/331443/67944711/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the reason seems to be that all object printers in pkg/kubectl/resource_printer.c seem to be operating on internal types unless I am missing something.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lavalamp all the registered column printers and describers work from internal types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

if sorter != nil {
mapping = infos[sorter.OriginalPosition(ix)].Mapping
original = infos[sorter.OriginalPosition(ix)].Object
} else {
mapping = infos[ix].Mapping
original = infos[ix].Object
}
if printer == nil || lastMapping == nil || mapping == nil || mapping.Resource != lastMapping.Resource {
printer, err = f.PrinterForMapping(cmd, mapping, allNamespaces)
Expand All @@ -275,12 +295,12 @@ func RunGet(f *cmdutil.Factory, out io.Writer, cmd *cobra.Command, args []string
lastMapping = mapping
}
if _, found := printer.(*kubectl.HumanReadablePrinter); found {
if err := printer.PrintObj(objs[ix], w); err != nil {
if err := printer.PrintObj(original, w); err != nil {
return err
}
continue
}
if err := printer.PrintObj(objs[ix], w); err != nil {
if err := printer.PrintObj(original, w); err != nil {
return err
}
}
Expand Down
50 changes: 50 additions & 0 deletions pkg/kubectl/cmd/get_test.go
View file
Open in desktop
Expand Up @@ -273,6 +273,56 @@ func TestGetObjects(t *testing.T) {
}
}

func TestGetSortedObjects(t *testing.T) {
pods := &api.PodList{
ListMeta: unversioned.ListMeta{
ResourceVersion: "15",
},
Items: []api.Pod{
{
ObjectMeta: api.ObjectMeta{Name: "c", Namespace: "test", ResourceVersion: "10"},
Spec: apitesting.DeepEqualSafePodSpec(),
},
{
ObjectMeta: api.ObjectMeta{Name: "b", Namespace: "test", ResourceVersion: "11"},
Spec: apitesting.DeepEqualSafePodSpec(),
},
{
ObjectMeta: api.ObjectMeta{Name: "a", Namespace: "test", ResourceVersion: "9"},
Spec: apitesting.DeepEqualSafePodSpec(),
},
},
}

f, tf, codec := NewAPIFactory()
tf.Printer = &testPrinter{}
tf.Client = &fake.RESTClient{
Codec: codec,
Resp: &http.Response{StatusCode: 200, Body: objBody(codec, pods)},
}
tf.Namespace = "test"
tf.ClientConfig = &restclient.Config{ContentConfig: restclient.ContentConfig{GroupVersion: &unversioned.GroupVersion{Version: "v1"}}}

buf := bytes.NewBuffer([]byte{})
cmd := NewCmdGet(f, buf)
cmd.SetOutput(buf)

// sorting with metedata.name
cmd.Flags().Set("sort-by", ".metadata.name")
cmd.Run(cmd, []string{"pods"})

// expect sorted: a,b,c
expected := []runtime.Object{&pods.Items[2], &pods.Items[1], &pods.Items[0]}
actual := tf.Printer.(*testPrinter).Objects
if !reflect.DeepEqual(expected, actual) {
t.Errorf("unexpected object: %#v", actual)
}
if len(buf.String()) == 0 {
t.Errorf("unexpected empty output")
}

}

func TestGetObjectsIdentifiedByFile(t *testing.T) {
pods, _, _ := testData()

Expand Down