Refactor the package structure in cmd/nerdctl #1639

Zheaoli · 2022-12-11T06:04:08Z

First, I'm sorry that I make this PR too large to review. I have to make a one-shot PR if I want to re-structured the directory because the code in the cmd/nerdctl has been mixed,

In this PR, I have made four things below

split the code file in the cmd/nerdctl into subdirectory which the subcommand has organized
make generic code into a common utils package in the cmd/nerdctl/utils
move the shell-completion feature into completion package in the cmd/nerdct/completion
move all the e2e test together.

Signed-off-by: Zheao.Li me@manjusaka.me

djdongjin

nit: integration might be a better name than e2e

Zheaoli · 2022-12-12T14:37:24Z

nit: integration might be a better name than e2e

nice catch(

AkihiroSuda · 2022-12-12T15:23:27Z

cmd/nerdctl/event/events.go

@@ -26,6 +26,8 @@ import (

 	"github.com/containerd/containerd/events"
 	"github.com/containerd/containerd/log"
+	client2 "github.com/containerd/nerdctl/cmd/nerdctl/client"


I'd prefer the package name to be like ncclient

Yes, I will clean dirty name when I pass all CI(lol

another suggestion is ndclient 😃 since nd reads more similar to nerdctl

I'd argue that the contaiNERD client library is also "nerd" 🤓

AkihiroSuda · 2022-12-12T15:24:12Z

cmd/nerdctl/event/events.go

@@ -26,6 +26,8 @@ import (

 	"github.com/containerd/containerd/events"
 	"github.com/containerd/containerd/log"
+	client2 "github.com/containerd/nerdctl/cmd/nerdctl/client"
+	fmt2 "github.com/containerd/nerdctl/cmd/nerdctl/utils"


I'd prefer the package name to be something like utils/fmtutil.

AkihiroSuda · 2022-12-12T15:26:17Z

cmd/nerdctl/create/create.go

@@ -14,16 +14,20 @@
   limitations under the License.
 */

-package main
+package create


Should this be in the nerdctl/container package, or maybe better in the nerdctl/container/create pkg?
Because the "canonical" form of nerdctl create is nerdctl container create.

Same applies to the run, exec, cp commands, etc.

Also, nerdctl info should be in nerdctl/system or nerdctl/system/info.

monirul · 2022-12-12T17:11:22Z

cmd/nerdctl/image/image_convert.go

@@ -30,6 +30,8 @@ import (
 	"github.com/containerd/containerd/images"
 	"github.com/containerd/containerd/images/converter"
 	"github.com/containerd/containerd/images/converter/uncompress"
+	client2 "github.com/containerd/nerdctl/cmd/nerdctl/client"


Maybe we can change either the package name (something like cliclient or nerdclient) or rename the variable client to avoid client2.

have already fix(

Revert unexpected changes in 5588f70 Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

AkihiroSuda · 2022-12-16T07:06:43Z

A bunch of functions seems misplaced in wrong files. (See my follow-up commits above)

I'm trying to fix them, but if I fail to do that by the end of the day, I'll prioritize merging other PRs.
But I fear that basically this PR isn't rebaseable after merging other PRs...

AkihiroSuda · 2022-12-16T07:13:00Z

And I also don't want to split the integration test package from the command packages

ktock

I'm trying to fix them, but if I fail to do that by the end of the day, I'll prioritize merging other PRs.
But I fear that basically this PR isn't rebaseable after merging other PRs...

Thank you for the following up. Possible plan B might be incrementally separating out cobra-independent functions into pkg directory but I'm not sure we need to do so because codes under cmd/nerdctl are tightly coupled. (cc @Zheaoli)

ktock · 2022-12-16T08:42:47Z

cmd/nerdctl/container/container_inspect.go

+func InspectAction(cmd *cobra.Command, args []string) error {
+	client, ctx, cancel, err := ncclient.New(cmd)


Exported functions should have godoc. In this case, XAction seems prints formatted strings to stdout so godoc should include the information about the format so that the caller can make sure it prints the expected outputs.
Maybe, exported functions should be independent from cobra and stdio.

ktock · 2022-12-16T08:54:35Z

cmd/nerdctl/utils/action/pause.go

+	"github.com/containerd/containerd/cio"
+)
+
+func PauseContainer(ctx context.Context, client *containerd.Client, id string) error {


It seems that we can put this under pkg dir because it doesn't have cobra dependency?

ktock · 2022-12-16T09:00:00Z

cmd/nerdctl/utils/fmtutil/fmtutil.go

-// formatSlice is expected to be only used for `nerdctl OBJECT inspect` commands.
-func formatSlice(cmd *cobra.Command, x []interface{}) error {
+// FormatSlice is expected to be only used for `nerdctl OBJECT inspect` commands.
+func FormatSlice(cmd *cobra.Command, x []interface{}) error {


If the purpose of the function is formatting rather than printing, I think it should receive io.Writer rather than *cobra.Command.

djdongjin · 2022-12-16T15:53:34Z

I'm trying to fix them, but if I fail to do that by the end of the day, I'll prioritize merging other PRs.
But I fear that basically this PR isn't rebaseable after merging other PRs...

Probably we can prioritize bugfix PRs which usually happens in pkg side, and wait a little bit on feature PRs?

AkihiroSuda · 2022-12-16T17:58:23Z

Sorry, I have to admit that I'm unable to review/carry this amount of PR 🙇‍♂️

Possible plan B might be incrementally separating out cobra-independent functions into pkg directory but I'm not sure we need to do so because codes under cmd/nerdctl are tightly coupled.

Yes, this should be done in a series of small PRs if we want to continue this.
e.g.,

PR 1: move cobra-independent functions into pkg/...
PR 2: split other common functions into somewhere like cmd/nerdctl/cmdpkg/...
PR 3: split the client cmdpkg
PR 4: split nerdctl namespace
PR 5: split nerdctl apparmor
PR 6: split nerdctl builder
PR 7: ...

The git diff has to be kept minimal for ease of reviewing and potential rebasing.
It mustn't contain unnecessary changes such as exposing private functions.
Some private functions could be exposed, but it has to be done in a separate PR.

Also, there shouldn't be the common package that is likely to result in being the home of random mutually irrelevant functions; the common package has to be split to multiple small cmdpkgs with clear purposes.

We also have to revisit whether we really have to split cmd/nerdctl in the first place ( #1631 (comment) ), as the functions in cmd/nerdctl aren't really designed to be imported from other projects.
There might be some functions that can be safely imported from third parties, but they should be rather decoupled from the CLI and should be moved to pkg/... .

Then there might be no good reason to split cmd/nerdctl ?
I expected that splitting cmd/nerdctl will increase the code readability as a side effect, but improper splitting will rather decrease the readability, so it has to be done seriously carefully.

@Zheaoli
Thanks anyway for your hard work, and hope you will get well soon! 👍

Zheaoli · 2022-12-16T18:24:59Z

Sorry, I have to admit that I'm unable to review/carry this amount of PR 🙇‍♂️

Possible plan B might be incrementally separating out cobra-independent functions into pkg directory but I'm not sure we need to do so because codes under cmd/nerdctl are tightly coupled.

Yes, this should be done in a series of small PRs if we want to continue this.

e.g.,

PR 1: move cobra-independent functions into pkg/...

PR 2: split other common functions into somewhere like cmd/nerdctl/cmdpkg/...

PR 3: split the client cmdpkg

PR 4: split nerdctl namespace

PR 5: split nerdctl apparmor

PR 6: split nerdctl builder

PR 7: ...

The git diff has to be kept minimal for ease of reviewing and potential rebasing.

It mustn't contain unnecessary changes such as exposing private functions.

Some private functions could be exposed, but it has to be done in a separate PR.

Also, there shouldn't be the common package that is likely to result in being the home of random mutually irrelevant functions; the common package has to be split to multiple small cmdpkgs with clear purposes.

We also have to revisit whether we really have to split cmd/nerdctl in the first place ( #1631 (comment) ), as the functions in cmd/nerdctl aren't really designed to be imported from other projects.

There might be some functions that can be safely imported from third parties, but they should be rather decoupled from the CLI and should be moved to pkg/... .

Then there might be no good reason to split cmd/nerdctl ?

I expected that splitting cmd/nerdctl will increase the code readability as a side effect, but improper splitting will rather decrease the readability, so it has to be done seriously carefully.

@Zheaoli

Thanks anyway for your hard work, and hope you will get well soon! 👍

I'm sorry for this huge PR, very sorry. It's my bad, the effort scope is beyond my think when I start this process.

I will close this PR first, and I will make a refactor proposal to design the refactor step.

Sorry again for this damn huge PR...

AkihiroSuda · 2022-12-16T18:26:43Z

Not "damn", don't blame your self ❤️

Zheaoli marked this pull request as draft December 11, 2022 06:04

Zheaoli force-pushed the manjusaka/support-1631 branch from 95f23b0 to b05df8f Compare December 11, 2022 06:04

AkihiroSuda added the kind/refactor label Dec 11, 2022

AkihiroSuda added this to the v1.1.1 (tentative) milestone Dec 11, 2022

Zheaoli force-pushed the manjusaka/support-1631 branch 5 times, most recently from 154b510 to 4d114ab Compare December 12, 2022 14:36

djdongjin reviewed Dec 12, 2022

View reviewed changes

Zheaoli force-pushed the manjusaka/support-1631 branch from 4d114ab to 50cc8b3 Compare December 12, 2022 14:41

Zheaoli marked this pull request as ready for review December 12, 2022 14:41

Zheaoli marked this pull request as draft December 12, 2022 14:42

Zheaoli force-pushed the manjusaka/support-1631 branch from 50cc8b3 to 3d2fcd9 Compare December 12, 2022 15:14

AkihiroSuda reviewed Dec 12, 2022

View reviewed changes

monirul reviewed Dec 12, 2022

View reviewed changes

Zheaoli force-pushed the manjusaka/support-1631 branch 2 times, most recently from 87fa51d to 8e52853 Compare December 12, 2022 18:40

Zheaoli marked this pull request as ready for review December 12, 2022 19:06

Zheaoli requested review from monirul, AkihiroSuda and djdongjin and removed request for monirul and djdongjin December 12, 2022 19:08

[Carry 1639] revert unexpected changes

e9f1417

Revert unexpected changes in 5588f70 Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

AkihiroSuda force-pushed the manjusaka/support-1631 branch from b46d661 to dc34976 Compare December 15, 2022 17:18

[Carry 1639] cmd/nerdctl: reorganize the package structure

206e0c8

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

AkihiroSuda force-pushed the manjusaka/support-1631 branch from dc34976 to 206e0c8 Compare December 15, 2022 23:04

AkihiroSuda added 4 commits December 16, 2022 09:21

[Carry 1639] nerdClient -> ncclient (avoid upper chars)

1116612

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

[Carry 1639] client.NewClient -> client.New

09cdb02

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

[Carry 1639] remove "Get" verb

4e03e4a

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

[Carry 1639] split cmd/nerdctl/utils/container

c0566a4

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

AkihiroSuda force-pushed the manjusaka/support-1631 branch from 48788c3 to c0566a4 Compare December 16, 2022 03:03

AkihiroSuda mentioned this pull request Dec 16, 2022

Support NewTerminal & Detached mode (nerdctl -t -d) #1642

Merged

AkihiroSuda added 7 commits December 16, 2022 15:02

[Carry 1639] move cmd/nerdctl/utils/{pause,unpause}.go

478ad56

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

[Carry 1639] move cmd/nerdctl/utils/mount.go

9bfbfa2

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

[Carry 1639] move cmd/nerdctl/utils/network.go

a1d9fa8

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

[Carry 1639] move cmd/nerdctl/utils/top.go

b00b5fe

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

[Carry 1639] fix the header of top.go

3b81f31

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

[Carry 1639] move cmd/nerdctl/utils/run

4284097

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

[Carry 1639] reorganize tests

3d2c7fd

Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>

ktock reviewed Dec 16, 2022

View reviewed changes

AkihiroSuda removed this from the v1.2.0 (tentative) milestone Dec 16, 2022

AkihiroSuda added the status/needs-design-discussion label Dec 16, 2022

AkihiroSuda mentioned this pull request Dec 16, 2022

Move *.go files for subcommand out main package #1631

Open

Zheaoli closed this Dec 16, 2022

Zheaoli deleted the manjusaka/support-1631 branch December 16, 2022 18:35

Zheaoli mentioned this pull request Dec 23, 2022

Let's refactor the nerdctl CLI package #1680

Open

83 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the package structure in cmd/nerdctl #1639

Refactor the package structure in cmd/nerdctl #1639

Zheaoli commented Dec 11, 2022 •

edited

Loading

djdongjin left a comment

Zheaoli commented Dec 12, 2022

AkihiroSuda Dec 12, 2022

Zheaoli Dec 12, 2022

djdongjin Dec 12, 2022

AkihiroSuda Dec 12, 2022

AkihiroSuda Dec 12, 2022

AkihiroSuda Dec 12, 2022 •

edited

Loading

monirul Dec 12, 2022

Zheaoli Dec 12, 2022

AkihiroSuda commented Dec 16, 2022 •

edited

Loading

AkihiroSuda commented Dec 16, 2022

ktock left a comment •

edited

Loading

ktock Dec 16, 2022

ktock Dec 16, 2022

ktock Dec 16, 2022

djdongjin commented Dec 16, 2022

AkihiroSuda commented Dec 16, 2022

Zheaoli commented Dec 16, 2022 •

edited

Loading

AkihiroSuda commented Dec 16, 2022

		func InspectAction(cmd *cobra.Command, args []string) error {
		client, ctx, cancel, err := ncclient.New(cmd)

Refactor the package structure in cmd/nerdctl #1639

Refactor the package structure in cmd/nerdctl #1639

Conversation

Zheaoli commented Dec 11, 2022 • edited Loading

djdongjin left a comment

Choose a reason for hiding this comment

Zheaoli commented Dec 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Dec 16, 2022 • edited Loading

AkihiroSuda commented Dec 16, 2022

ktock left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djdongjin commented Dec 16, 2022

AkihiroSuda commented Dec 16, 2022

Zheaoli commented Dec 16, 2022 • edited Loading

AkihiroSuda commented Dec 16, 2022

Zheaoli commented Dec 11, 2022 •

edited

Loading

AkihiroSuda Dec 12, 2022 •

edited

Loading

AkihiroSuda commented Dec 16, 2022 •

edited

Loading

ktock left a comment •

edited

Loading

Zheaoli commented Dec 16, 2022 •

edited

Loading