[Refactor] Refactor the compose command flagging process#1818
Conversation
|
Refactor the After (#1797) merged, this pr should be merged. |
| @@ -82,135 +69,44 @@ func newComposeCommand() *cobra.Command { | |||
| } | |||
|
|
|||
| func getComposer(cmd *cobra.Command, client *containerd.Client, globalOptions *types.GlobalCommandOptions) (*composer.Composer, error) { | |||
There was a problem hiding this comment.
After the below suggestion, this getComposer becomes a wraper of 2 function calls: getComposeOptions in cmd side and GetComposer in pkg side.
Although it seems redundant, I think it's fine for now to keep it to minimize the change. Otherwise, you need to change every getCompoer call in compose commands to first call getComposeOptions and then call compose.GetComposer.
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| func GetComposer(options *composer.Options, globalOptions *types.GlobalCommandOptions, client *containerd.Client, volStore volumestore.VolumeStore, stdout, stderr io.Writer) (*composer.Composer, error) { |
There was a problem hiding this comment.
You can put this function here because most pkg side compose logic are here. And we might need some further refactor on GetComposer and composer.New (GetComposer is basically a New with options).
Laitr0n
force-pushed
the
refactor_compose
branch
3 times, most recently
from
a44f517 to
7fe89c3
Compare
| options.Experimental, err = cmd.Flags().GetBool("experimental") | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
I think the original format of the first half function should serve us well right? (e.g., read cmd flags, and create composer.Options altogether) (so suggest not change the 1st half)
Meanwhile, as you can see in this whole function, it contains two types of code:
- cobra related flag process: it's not related to compose implementation details so we should keep it in
cmdside; - compose implementation details: e.g., those
o.NetworkExists = ..., o.VolumeExists = ...etc. they are not related to cobra/cmd after we get the option values from the 1st part.
What I meant in my comment (#1791 (comment)) is that we should spit this two parts into two functions (so cobra logic stays in cmd and implementation goes to pkg):
getComposeOptions(cmd *cobra.Command) composer.Options: basically the 1st half. after this call,composer.Optionscontains all option values needed for the implementation.GetComposer(opt composer.Options, client *containerd.Client, stdout, stderr io.Writer) (*composer.Composer, error): basically the 2nd half. It first adds (our default) implementation tooptionand then returns acomposer. If we want to enable callers to provide other implementations viacomposeoption, we should check if the field if nil.
if o.EnsureImage == nil {
o.EnsureImage = func(....
}After this change, the workflow for a compose command to obtain a composer will be:
// get option values from cmd
options, err := getComposeOptions(cmd, client, globalOptions)
if err != nil {
return err
}
// get composer
c, err := composer.GetComposer(options, client, cmd.OutOrStdout(), cmd.ErrOrStdErr())Let me know if you have other questions. @Laitr0n
@AkihiroSuda, @Zheaoli WDYT? (does this make sense :))
Laitr0n
force-pushed
the
refactor_compose
branch
3 times, most recently
from
3f08056 to
cfe0acf
Compare
| options := &composer.Options{} | ||
| options.NerdctlCmd, options.NerdctlArgs = globalFlags(cmd) | ||
| var err error | ||
| options.ProjectDirectory, err = cmd.Flags().GetString("project-directory") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| envFile, err := cmd.Flags().GetString("env-file") | ||
| options.EnvFile, err = cmd.Flags().GetString("env-file") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| projectName, err := cmd.Flags().GetString("project-name") | ||
| options.Project, err = cmd.Flags().GetString("project-name") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| debugFull := globalOptions.DebugFull | ||
| snapshotter := globalOptions.Snapshotter | ||
| files, err := cmd.Flags().GetStringArray("file") | ||
| options.DebugPrintFull, err = cmd.Flags().GetBool("debug-full") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| insecure := globalOptions.InsecureRegistry | ||
| cniPath := globalOptions.CNIPath | ||
| cniNetconfpath := globalOptions.CNINetConfPath | ||
| hostsDirs := globalOptions.HostsDir | ||
| experimental := globalOptions.Experimental | ||
|
|
||
| o := composer.Options{ | ||
| Project: projectName, | ||
| ProjectDirectory: projectDirectory, | ||
| ConfigPaths: files, | ||
| EnvFile: envFile, | ||
| NerdctlCmd: nerdctlCmd, | ||
| NerdctlArgs: nerdctlArgs, | ||
| DebugPrintFull: debugFull, | ||
| Experimental: experimental, | ||
| } |
There was a problem hiding this comment.
can we keep this first half not changed (e.g., return o, nil directly here)? This part is only syntax changes and we can just do the split it into 2 functions. It makes the PR easy to review.
There was a problem hiding this comment.
And the return type can be composer.Options (i.e., not a pointer)
Laitr0n
force-pushed
the
refactor_compose
branch
2 times, most recently
from
d66d864 to
520c421
Compare
Signed-off-by: Laitron <meetlq@outlook.com>
Laitr0n
force-pushed
the
refactor_compose
branch
from
520c421 to
b07417f
Compare
3 participants
Signed-off-by: Laitron meetlq@outlook.com