Let's refactor the nerdctl CLI package #1680

Zheaoli · 2022-12-23T10:03:03Z

Update in 2022-12-26

After some discussion on this issue, most of the nerdctl maintainers reach the same point.

We need to refactor the command flagging process. After the refactor process, all logic in cmd package should only validate cobra flags and construct the CommandOption.

Refactor Check List:

Create a file in pkg/api/types/${cmd}_types.go, and define the CommandOption for this command
Create some file in pkg/cmd/${cmd}, and move the command entry point in real into this package
~~[ ] Create new docs in docs/reference/api/${cmd}_types.md, and describe the field in CommandOption~~

I will create single issue for each subcommand

What is the problem you're trying to solve

OK, I'm back!

For now, the cmd package has a lot of go files mixed. It takes a lot of work to keep maintainer to maintain this project.

And in the community, people may need to add some features of their own, like #1631, so it's time to refactor the command package!

Describe the solution you'd like

I have made a previous PR, #1639; it's a nasty case for the community.

We may need to make some changes to make this issue happen.

I want to split this issue into three stages.

First: we need to decompose the flagging process in the command entry point

For now, we mix the flagging register and flagging process, like https://github.com/containerd/nerdctl/blob/main/cmd/nerdctl/exec.go#L53-L62

It means we can not split the common logic unrelated to cobra.Command, and care about the flag value from the current cmd package.

We can take a look at some projects in the community alike https://github.com/prometheus/prometheus/blob/main/cmd/prometheus/main.go#L127-L159 and
https://github.com/containers/podman/blob/main/cmd/podman/containers/attach.go#L45-L54, we will find out that we may collect the flag value into a centralize struct, so we do not need care about the cobra.Command in the entry point in real.

Second: split the common function into another package

For now, the cmd package maybe can be grouped into three types.

helper function, like https://github.com/containerd/nerdctl/blob/main/cmd/nerdctl/client.go
shellCompletion , like https://github.com/containerd/nerdctl/blob/main/cmd/nerdctl/client.go
the logic function, mixed with the flag process logic, like https://github.com/containerd/nerdctl/blob/main/cmd/nerdctl/apparmor_ls_linux.go#L56

In this stage, we need to do three things.

make the helper function out of the cmd package
make the shellCompletion together
after stage1, we can split the logic function into the pkg/ (for now, we don't need care about the cobra action in the logic function now!)

Third: restructed the cmd package by the subcommand

And now, we can restruct the cmd package by subcommand. The code in the cmd package has already been cleaned and will focus on the logic to process the flag. So the restruct process will be easy.

And the final problem in this stage the integration test would be a problem. For most circumstances, the integration test may be written with the command. But here's some in issue in go test, the go test command will run the test in different directory parallel, so there would be some potential conflict in this circumstance. I would prefer to make all the integration tests into a single directory together.

Additional context

No response

The text was updated successfully, but these errors were encountered:

AkihiroSuda · 2022-12-23T12:36:21Z

Before working on this we have to revisit whether we really need to split the pkgs, and what are potential drawbacks.

I would prefer to make all the integration tests into a single directory together.

I'd expect the subcommand.go and the subcommand_test.go to be placed in the same directory, otherwise contributors can't find where to add tests.

Zheaoli · 2022-12-23T12:48:34Z

Before working on this we have to revisit whether we really need to split the pkgs, and what are potential drawbacks

Totally agree, but I think the stage1(refactoring the flag is needed whether we need to split it or not)

I'd expect the subcommand.go and the subcommand_test.go to be placed in the same directory, otherwise contributors can't find where to add tests.

I'm not sure if there is any way to run the go test in a non-parallel way. There will be some conflict in here, for example, we split the compose and the container * in different sub-command packages. The tests which open the same 80 local port will be run at the same time, so one of the tests will be failed because of the port conflict

djdongjin · 2022-12-23T20:05:51Z

I think maybe we can first focus more on the 1st step, which itself will significantly simplify our codebase by drawing a clear separation between flag/cobra processing and core logic processing.

Then we can have better sense on how to proceed to next steps (move logic functions to approriate packages) because they will be more modularized and not depends on flags/cobra.

I agree each command should have a CommandOption struct, regardless small or large. We should put them in a separate pkg (e.g., api/types.go, or pkg/api/types.go).
Then cmd/command.go should only validate cobra flags and construct the CommandOption, then pass it to pkg/command.go (or at least to a separate process func that accept CommandOption).

Most nerdctl compose related commands follows this pattern, and I found it's more clear and easy to find where the real logic is.

Although even doing 1st step might also involve moving some helper functions, probably that shouldn't be the main focus during this stage.

Zheaoli · 2022-12-24T14:59:47Z

I agree each command should have a CommandOption struct, regardless small or large. We should put them in a separate pkg (e.g., api/types.go, or pkg/api/types.go).

Then cmd/command.go should only validate cobra flags and construct the CommandOption, then pass it to pkg/command.go (or at least to a separate process func that accept CommandOption).

Totally agree, maybe we can refactor this subcommand by subcommand cc @AkihiroSuda @fahedouch

If we reach a consensus, I will split step1 into multi-subtask(most of the subtasks would be great good-first-issue(lol

fahedouch · 2022-12-25T16:13:21Z

Welcome back @Zheaoli 👋,

I agree each command should have a CommandOption struct, regardless small or large. We should put them in a separate pkg (e.g., api/types.go, or pkg/api/types.go).

I like the idea of having pkg/apis/ that contains the API resource definitions. We can split api files based on corresponding feature type:

pkg/apis/run_types.go
pkg/apis/container_management_types.go
pkg/apis/image_management_types.go
pkg/apis/compose_types.go
pkg/apis/stats_types.go
etc..

So contributors edit the *_types.go files to implement their API definitions. ( I was inspired by kubebuilder Project Creation and Structure ).

Then cmd/command.go should only validate cobra flags and construct the CommandOption, then pass it to pkg/command.go (or at least to a separate process func that accept CommandOption).

agreed, this will improve code readability.

refactor this subcommand by subcommand

I think before thinking about refactoring subcommand(s) , we need to ask :

do we really have to split subcommands by subcommand ?

As the subcommands themselves aren't really designed to be imported from other projects. Every function related to subcommands and need to be imported should be under pkg/...
Then there might be no good reason to split cmd/nerdctl ?

Now regarding *_test.go. I found more practical to have subcommand.go and the subcommand_test.go to be placed in the same directory. Otherwise, it will be very time consuming.

Zheaoli · 2022-12-25T17:48:05Z

Welcome back @Zheaoli wave,

Thanks(lol

I like the idea of having pkg/apis/ that contains the API resource definitions. We can split api files based on corresponding feature type:

pkg/apis/run_types.go

pkg/apis/container_management_types.go

pkg/apis/image_management_types.go

pkg/apis/compose_types.go

pkg/apis/stats_types.go

etc..

So contributors edit the *_types.go files to implement their API definitions. ( I was inspired by kubebuilder Project Creation and Structure ).

Then cmd/command.go should only validate cobra flags and construct the CommandOption, then pass it to pkg/command.go (or at least to a separate process func that accept CommandOption).

agreed, this will improve code readability.

It seems we have reached the same point. we need to refactor the codebase to split the CommandOptiton and the flagging process"! We can discuss this later after we finish the flagging refactor step. In my thought, if we can make the Then cmd/command.go should only validate cobra flags and construct the CommandOption, we may don't need to split the cmd package

Zheaoli · 2022-12-25T17:51:16Z

What are other people's thoughts? @containerd/nerdctl-reviewers @containerd/nerdctl-committers

AkihiroSuda · 2022-12-25T23:16:04Z

Then cmd/command.go should only validate cobra flags and construct the CommandOption, we may don't need to split the cmd package

SGTM

yuchanns · 2022-12-27T02:56:35Z

Somewhat confused about a bunch of closed issues presented in your description. If you're going to add a number of good first issues, I would like to suggest that you leave only tasks here and ppl can open issues by themselves. As for tracking, it's enough for others simply make references to the issue.

miles170 · 2022-12-27T04:00:59Z

If I want to contribute one of them, should I open issue first or just creates a PR?

yuchanns · 2022-12-27T04:07:53Z

@miles170 a PR is enough IMO.

Refactor the loading and application of the networking-related arguments for the `nerdctl run` command in order to facilitate Windows support. Fixes/alleviates containerd#559 and containerd#1680. Signed-off-by: Nashwan Azhari <nazhari@cloudbasesolutions.com>

arnaldo2792 · 2023-02-09T23:15:22Z

I want to help with this, but I'm confused with the current state. Are the items that are not checked available for grabs? ❓

fahedouch · 2023-02-16T10:14:38Z

@arnaldo2792

I want to help with this, but I'm confused with the current state. Are the items that are not checked available for grabs? ❓

not sure but you can verify by checking the attached PR(s) shown in this ticket.

caioluis · 2023-03-26T04:53:12Z

Hi people! I was having a discussion at Infisical (Infisical/infisical#425 (comment)). That discussion brought me to this discussion that is being discussed for 9 years!

Could we implement somehow passing individual env variables (in the format: -e VAR=VALUE) or env files on docker start or docker compose? I know that the nerdctl compose we currently have HAS --env-file flag, but could we extend this to single env vars as well (-e), for both nerdctl start and nerdctl compose (for up, start)?

I think it could be a great thing! And I also could help implement this and also implement the integration for Infisical as well!

Anutrix · 2024-01-13T11:08:36Z

Is this done?

Zheaoli added status/needs-design-discussion kind/refactor labels Dec 23, 2022

Zheaoli added this to the v1.4.0 milestone Dec 26, 2022

Zheaoli added good first issue Good for newcomers and removed status/needs-design-discussion labels Dec 26, 2022

Zheaoli pinned this issue Dec 26, 2022

yuchanns mentioned this issue Dec 27, 2022

Refactor the compose ps command flagging process #1702

Closed

HanquanHq mentioned this issue Dec 27, 2022

Refactor the compose ps command flagging process #1770

Closed

miles170 mentioned this issue Dec 27, 2022

Refactor the volume ls command flagging process #1773

Merged

2 tasks

djdongjin mentioned this issue Dec 27, 2022

Refactor the apparmor flagging process #1774

Merged

2 tasks

Zheaoli mentioned this issue Dec 29, 2022

[Refactor] Move containerd client helper from cmd to pkg/clientutil #1780

Merged

miles170 mentioned this issue Jan 3, 2023

Refactor the volume flagging process #1795

Merged

2 tasks

miles170 mentioned this issue Jan 10, 2023

Refactor the network command flagging process #1822

Merged

2 tasks

djdongjin mentioned this issue Jan 12, 2023

Refactor nerdctl rm #1834

Merged

Laitr0n mentioned this issue Jan 12, 2023

[Refactor] Refactor the kill command flagging process #1836

Merged

2 tasks

weikequ mentioned this issue Jan 13, 2023

Refactor container inspect #1850

Merged

2 tasks

yzxiu mentioned this issue Jan 14, 2023

[Refactor] Refactor network list #1851

Merged

2 tasks

suyanhanx mentioned this issue Jan 14, 2023

[Refactor] Refactor images pull command #1852

Merged

2 tasks

yzxiu mentioned this issue Jan 17, 2023

[Refactor] Refactor network inspect #1860

Closed

2 tasks

davidhsingyuchen mentioned this issue Jan 24, 2023

refactor: system events flagging process #1916

Merged

2 tasks

This was referenced Jan 24, 2023

[Refactor] Refactor the (container) pause command flagging process #1920

Merged

[WIP][Refactor] Refactor the (container) run command flagging process #1921

Closed

suyanhanx mentioned this issue Jan 25, 2023

[Refactor] refactor image save flagging process #1924

Merged

2 tasks

MimeLyc mentioned this issue Jan 26, 2023

refactor(container): refactor cp flag #1930

Merged

2 tasks

This was referenced Jan 26, 2023

Make image prune remove dangling images by default #1922

Closed

[Refactor] refactor image prune flag #1931

Merged

This was referenced Jan 27, 2023

refactor: container wait flagging process #1934

Merged

refactor: move unpackedImageSize to imgutil #1936

Merged

aznashwan mentioned this issue Jan 30, 2023

Refactor run command networking options for Windows support. #1953

Merged

djdongjin mentioned this issue Feb 2, 2023

Refactor (un)pause in nerdctl and compose #1965

Merged

davidhsingyuchen mentioned this issue Feb 2, 2023

refactor: container list flagging process #1966

Merged

2 tasks

suyanhanx mentioned this issue Feb 8, 2023

[Refactor] refactor container prune flagging process #1984

Closed

djdongjin mentioned this issue Feb 9, 2023

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

Open

rkonfj mentioned this issue Feb 9, 2023

Refactor container prune and rename command flagging process #1994

Merged

rkonfj mentioned this issue Feb 11, 2023

Refactor container ps command flagging process #2003

Closed

AkihiroSuda modified the milestones: v1.3.1, v1.4.0 (tentative) Apr 13, 2023

AkihiroSuda removed this from the v1.4.0 milestone May 18, 2023

mharwani mentioned this issue May 30, 2023

Using a custom exe for logging in refactored container create #2264

Open

ningziwen mentioned this issue Jun 15, 2023

refactor: container stats command #2306

Merged

ningziwen mentioned this issue Jun 27, 2023

refactor: extract the printing logic out of pkg for container list #2333

Merged

mharwani mentioned this issue Jun 30, 2023

refactor: allow custom handlers for processing push/pull status updates #2342

Open

ningziwen mentioned this issue Sep 5, 2023

feat: support push with soci #2467

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let's refactor the nerdctl CLI package #1680

Let's refactor the nerdctl CLI package #1680

Zheaoli commented Dec 23, 2022 •

edited by junnplus

AkihiroSuda commented Dec 23, 2022

Zheaoli commented Dec 23, 2022

djdongjin commented Dec 23, 2022 •

edited

Zheaoli commented Dec 24, 2022 •

edited

fahedouch commented Dec 25, 2022 •

edited

Zheaoli commented Dec 25, 2022 •

edited

Zheaoli commented Dec 25, 2022

AkihiroSuda commented Dec 25, 2022

yuchanns commented Dec 27, 2022 •

edited

miles170 commented Dec 27, 2022

yuchanns commented Dec 27, 2022

arnaldo2792 commented Feb 9, 2023

fahedouch commented Feb 16, 2023 •

edited

caioluis commented Mar 26, 2023

Anutrix commented Jan 13, 2024

Let's refactor the nerdctl CLI package #1680

Let's refactor the nerdctl CLI package #1680

Comments

Zheaoli commented Dec 23, 2022 • edited by junnplus

Update in 2022-12-26

What is the problem you're trying to solve

Describe the solution you'd like

First: we need to decompose the flagging process in the command entry point

Second: split the common function into another package

Third: restructed the cmd package by the subcommand

Additional context

AkihiroSuda commented Dec 23, 2022

Zheaoli commented Dec 23, 2022

djdongjin commented Dec 23, 2022 • edited

Zheaoli commented Dec 24, 2022 • edited

fahedouch commented Dec 25, 2022 • edited

Zheaoli commented Dec 25, 2022 • edited

Zheaoli commented Dec 25, 2022

AkihiroSuda commented Dec 25, 2022

yuchanns commented Dec 27, 2022 • edited

miles170 commented Dec 27, 2022

yuchanns commented Dec 27, 2022

arnaldo2792 commented Feb 9, 2023

fahedouch commented Feb 16, 2023 • edited

caioluis commented Mar 26, 2023

Anutrix commented Jan 13, 2024

Zheaoli commented Dec 23, 2022 •

edited by junnplus

djdongjin commented Dec 23, 2022 •

edited

Zheaoli commented Dec 24, 2022 •

edited

fahedouch commented Dec 25, 2022 •

edited

Zheaoli commented Dec 25, 2022 •

edited

yuchanns commented Dec 27, 2022 •

edited

fahedouch commented Feb 16, 2023 •

edited