-
Notifications
You must be signed in to change notification settings - Fork 592
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.
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
Let's refactor the nerdctl CLI package #1680
Comments
Before working on this we have to revisit whether we really need to split the pkgs, and what are potential drawbacks.
I'd expect the |
Totally agree, but I think the stage1(refactoring the flag is needed whether we need to split it or not)
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 |
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.
Most
|
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 like the idea of having
So contributors edit the
agreed, this will improve code readability.
I think before thinking about refactoring
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 Now regarding |
Thanks(lol
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 |
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. |
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 <[email protected]>
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 <[email protected]>
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 (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: 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
Curious if anyone remembers what was the issue with _tests with splitting 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 |
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 package[ ] Create new docs indocs/reference/api/${cmd}_types.md
, and describe the field in CommandOptionI 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
The text was updated successfully, but these errors were encountered: