Refactor the apparmor flagging process

[Zheaoli]

CheckList:

• Create a file in pkg/api/types/${cmd}.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

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

[AkihiroSuda]

Thanks, this one is easily reviewable 👍

[fahedouch]

I wonder if we should put /cmd in the same level as pkg/apis since apis folder generally only contains API resource definitions. IMHO pkg/cmd/${cmd} fit better.

cc @AkihiroSuda @Zheaoli

[djdongjin]

Create a file in pkg/api/types/${cmd}/types.go, and define the CommandOption for this command

I don't think creating a sub-package (pkg/api/types/cmd) for each command (or each command type, e.g., image, container, compose) is necessary. I prefer including them in a single package (e.g., pkg/api/types or just pkg/api) and prefix them (either pkg/api/types/apparmor_types.go or pkg/api/apparmor_types.go as @fahedouch suggested in #1680 (comment)).

Create some file in pkg/api/cmd/${cmd}, and move the command entry point in real into this package

Why these entry point functions need to be in pkg/api? As an example, similar function for compose are located here: https://github.com/containerd/nerdctl/tree/main/pkg/composer

[djdongjin]

Just a question, no action requested :) :

I wonder, while we move backend logic into pkg side, should we also gradually collocate those backend logic and the specific util package into one package? Take apparmor as an example:

1. we will have pkg/cmd/apparmor (or any pkg/.../apparmor decieded) for the backend logic.
2. we have pkg/apparmorutil for main functions related to apparmor.

I guess most functions in pkg/cmd/apparmor will be just take and (post)process some ApparmorOption fields and then call a function in pkg/apparmorutil.

cc @Zheaoli @AkihiroSuda @fahedouch

[Zheaoli]

I think this xxutil should only contain some pure helper function. The most important logic shouble be completed in the pkg/xxx/{cmd}

[Zheaoli]

I wonder if we should put /cmd in the same level as pkg/apis since apis folder generally only contains API resource definitions. IMHO pkg/cmd/${cmd} fit better.

We still need a package to collect the CommandOption. I'm not sure if we collect all the data structure and logic together is a good idea

[Zheaoli]

I don't think creating a sub-package (pkg/api/types/cmd) for each command (or each command type, e.g., image, container, compose) is necessary. I prefer including them in a single package (e.g., pkg/api/types or just pkg/api) and prefix them (either pkg/api/types/apparmor_types.go or pkg/api/apparmor_types.go as @fahedouch suggested in #1680 (comment)).

I still think we need some subpackage in pkg/api/types/cmd, because the CommandOption may be split into some split file base on the command type(one file for all of the data structure will make this file too large to read and get more conflict in this cooperated refactor process

[djdongjin]

I still think we need some subpackage in pkg/api/types/cmd, because the CommandOption may be split into some split file base on the command type(one file for all of the data structure will make this file too large to read and get more conflict in this cooperated refactor process

Not necessarily one file, but organize by subpackage seems adding too much hierarchy. We can still split those CommandOption into different files but within the same package (e.g., pkg/api/types)

1. It's more aligned with how we organize cmd package (a flat package with all command files, with prefix when necessary).
2. Easy to navigate: all Option structs are under pkg/api/types folder/package, don't need to go one level deeper, just check the file name within pkg/api/types.
3. More user-friendly: if we organize them by subpackages and a downstream library wants to use our resource definitions, they only need to import a single pkg/api/types, instead of a list of pkg/api/types/commandA, pkg/api/types/commandB.
4. Organizing by sub-packages will add unnecessary dependency: for example, the package with RunOption will depend on the package with CreateOption (https://github.com/containerd/nerdctl/blob/main/cmd/nerdctl/run.go#L89-L91), etc.

[Zheaoli]

1. It's more aligned with how we organize cmd package (a flat package with all command files, with prefix when necessary).

I prefer multi-level package. But you are right, keeping the flatten style with cmd package is more critical.

So I think we reach one point

1. all CommandOption should be placed in pkg/api/types

[Zheaoli]

And the second problem, pkg/cmd/{cmd} or pkg/{cmd} like pkg/compose

[fahedouch]

1. It's more aligned with how we organize cmd package (a flat package with all command files, with prefix when necessary).

I prefer multi-level package. But you are right, keeping the flatten style with cmd package is more critical.

So I think we reach one point

1. all CommandOption should be placed in pkg/api/types

Happy to see things going ahead 😀. I propose to stay in the Middle, So we keep the flat package as @djdongjin said and Split by category :

- pkg/apis/run_types.go
- pkg/apis/container_management_types.go
- pkg/apis/image_management_types.go

So we :

• ensure medium file size (avoid having Huge or very small files) for better contributing experience.
• Easy to manipulate; contributors have functionnel related data structures in the same file. Even mentally, it's easier to make the connection between the structures in the same file

And the second problem, pkg/cmd/{cmd} or pkg/{cmd} like pkg/compose

pkg/cmd/{cmd to avoid polluting the pkg folder

[AkihiroSuda]

Could you consider splitting the fmtutil commit

[Zheaoli]

Could you consider splitting the fmtutil commit

This PR should be merged after #1779

[Zheaoli]

#1779 has been merged, I think this PR is ok to merge

[AkihiroSuda]

Thanks