fix: inspect should return one array rather than a stream of array #3961

weiyuhang2011 · 2025-03-03T08:53:54Z

Description

This PR fixes an issue where nerdctl inspect <container1> <image1> return a stream of array rather than one array.

Steps to reproduce the issue

current

> nerdctl inspect container1 image1
[
    {
       <container1-info>
    }
]
[
    {
       <image1-info>
    }
]

fixed

> nerdctl inspect container1 image1
[
    {
       <container1-info>
    },
    {
       <image1-info>
    }
]

Fix: #3006

AkihiroSuda · 2025-03-03T08:55:20Z

Thanks, can we have a test?

weiyuhang2011 · 2025-03-03T09:12:07Z

Thanks, can we have a test?

Sure, i will add a test.

manugupt1 · 2025-04-08T02:18:14Z

@weiyuhang2011 Ping! Did you find time to write tests? Thank you!

weiyuhang2011 · 2025-04-08T02:36:30Z

@weiyuhang2011 Ping! Did you find time to write tests? Thank you!

Indeed, it's been delayed for quite a while... I've prepared an initial draft and will strive to complete it this week.

cmd/nerdctl/inspect/inspect_test.go

pkg/cmd/container/inspect.go

pkg/cmd/image/inspect.go

apostasie · 2025-04-13T21:24:24Z

I do personally like that we would be moving presentation concerns into ./cmd/nerdctl instead of having them in ./pkg/cmd/nerdctl.
I think it is just better for people who want to consume nerdctl as a library - as presentation should be left as a concern to the final cli. Generally speaking, I would like to see any Fprintf gone from the "library" part. Right now, ./pkg/cmd is an unfortunate mix of data processing and display.

That being said, this is personal opinion, and I am not sure if this was discussed before and/or if there was agreement on this.

apostasie · 2025-04-13T21:32:28Z

cmd/nerdctl/container/container_inspect.go

+	}
+
+	// Display
+	if len(entries) > 0 {


Is this necessary?

i think we can keep it, but remove the one in cmd/nerdctl/inspect/inspect.go

apostasie · 2025-04-13T21:34:10Z

cmd/nerdctl/image/image_inspect.go

+	}
+
+	// Display
+	if len(entries) > 0 {


Same question as above.

apostasie · 2025-04-13T21:37:27Z

Two main comments:

interface{} -> any
do we need to check on len(entries) > 0?

Plus a nit inside test.

Otherwise LGTM.

djdongjin

LGTM

Signed-off-by: Yuhang Wei <weiyuhang2011@gmail.com>

`nerdctl inspect <container> <image>` should return a single JSON array containing both the container and image inspect results. This unit test parses the output and compares it with the results of separate `inspect` commands to ensure correctness. Signed-off-by: Yuhang Wei <weiyuhang2011@gmail.com>

weiyuhang2011 · 2025-04-14T13:58:36Z

It seems like the CI is failing on some tests not relevant to this PR?

AkihiroSuda

Thanks

Next time please consider squashing the commits

AkihiroSuda added this to the v2.0.4 milestone Mar 3, 2025

AkihiroSuda modified the milestones: v2.0.4, v2.x.x Mar 18, 2025

weiyuhang2011 force-pushed the fix-inspect branch 2 times, most recently from 3ce4a1a to 36c24b5 Compare April 12, 2025 12:52

manugupt1 approved these changes Apr 13, 2025

View reviewed changes

manugupt1 requested a review from a team April 13, 2025 21:13

manugupt1 modified the milestones: v2.x.x, v2.0.5 Apr 13, 2025