Open Zheaoli opened 1 year ago
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.
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 thesubcommand_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
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.
CommandOption
struct, regardless small or large. We should put them in a separate pkg (e.g., api/types.go
, or pkg/api/types.go
).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.
- 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
, orpkg/api/types.go
).- Then
cmd/command.go
should only validate cobra flags and construct theCommandOption
, then pass it topkg/command.go
(or at least to a separate process func that acceptCommandOption
).
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
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
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.
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
What are other people's thoughts? @containerd/nerdctl-reviewers @containerd/nerdctl-committers
Then cmd/command.go should only validate cobra flags and construct the CommandOption, we may don't need to split the cmd package
SGTM
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.
If I want to contribute one of them, should I open issue first or just creates a PR?
@miles170 a PR is enough IMO.
I want to help with this, but I'm confused with the current state. Are the items that are not checked available for grabs? :question:
@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.
Hi people! I was having a discussion at Infisical (https://github.com/Infisical/infisical/issues/425#issuecomment-1483253631). 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!
Is this done?
Hey folks,
Coming from #3375
Would we be open to putting that stuff under sub-folders, by category? (container, image, network, volume, system, etc) IIRC we have tried this before and faced an issue about splitting *_test.go
Curious if anyone remembers what was the issue with _tests with splitting cmd/nerdctl
.
I do have a private branch that does that (along with many other changes, so, not anywhere near a PR) ( https://github.com/apostasie/nerdctl/tree/lepton/cmd/nerdctl )
I got tests working for volumes
at this point with no specific difficulty - but maybe there are difficulties awaiting in less clean packages - so wondering what people think.
Update in 2022-12-26
After some discussion on this issue, most of the nerdctl maintainers reach the same point.
Refactor Check List:
pkg/api/types/${cmd}_types.go
, and define the CommandOption for this commandpkg/cmd/${cmd}
, and move the command entry point in real into this packagedocs/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.
In this stage, we need to do three things.
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
, thego 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