Skip to content

Refactor the compose build command flagging process#1791

Closed
Laitr0n wants to merge 2 commits into
containerd:mainfrom
Laitr0n:refactot_compose_build
Closed

Refactor the compose build command flagging process#1791
Laitr0n wants to merge 2 commits into
containerd:mainfrom
Laitr0n:refactot_compose_build

Conversation

@Laitr0n
Copy link
Copy Markdown
Contributor

@Laitr0n Laitr0n commented Jan 2, 2023

There is no ideal about GlobalFlag(like namespace, address). I just refactor the compose build command.
Signed-off-by: Laitron meetlq@outlook.com

@Laitr0n Laitr0n marked this pull request as ready for review January 2, 2023 14:55
@Laitr0n Laitr0n marked this pull request as draft January 2, 2023 15:05
@Laitr0n Laitr0n force-pushed the refactot_compose_build branch from e396e1e to a9b2bcc Compare January 2, 2023 15:09
@Zheaoli
Copy link
Copy Markdown
Member

Zheaoli commented Jan 2, 2023

Maybe you can update this pr after #1792 has been merged. I introduce a new global flag process helper function in #1792. it may help you

@Laitr0n Laitr0n force-pushed the refactot_compose_build branch from a9b2bcc to 32ec97f Compare January 3, 2023 15:47
@Laitr0n
Copy link
Copy Markdown
Contributor Author

Laitr0n commented Jan 3, 2023

Maybe you can update this pr after #1792 has been merged. I introduce a new global flag process helper function in #1792. it may help you

After merge pr #1793, Should we refactor

func NewClient(ctx context.Context, namespace, address string, opts ...containerd.ClientOpt) (*containerd.Client, context.Context, context.CancelFunc, error) 

to

func NewClient(commonOpts types.CommonOptions, GOptions * types.GlobalCommandOptions, opts ...containerd.ClientOpt) (*containerd.Client, context.Context, context.CancelFunc, error) 

first? Composer subcommand use this func many times, and maybe we refactor them one more time if not refactor it firstly.

@Zheaoli
Copy link
Copy Markdown
Member

Zheaoli commented Jan 3, 2023

Maybe you can update this pr after #1792 has been merged. I introduce a new global flag process helper function in #1792. it may help you

After merge pr #1793, Should we refactor


func NewClient(ctx context.Context, namespace, address string, opts ...containerd.ClientOpt) (*containerd.Client, context.Context, context.CancelFunc, error) 

to


func NewClient(commonOpts types.CommonOptions, GOptions * types.GlobalCommandOptions, opts ...containerd.ClientOpt) (*containerd.Client, context.Context, context.CancelFunc, error) 

first? Composer subcommand use this func many times, and maybe we refactor them one more time if not refactor it firstly.

Nice catch, SGTM, I will make a PR tonight.

@djdongjin
Copy link
Copy Markdown
Member

djdongjin commented Jan 3, 2023

func NewClient(commonOpts types.CommonOptions, GOptions * types.GlobalCommandOptions, opts ...containerd.ClientOpt) (*containerd.Client, context.Context, context.CancelFunc, error) 
  1. What is commonOpts types.CommonOptions?
  2. If NewClient only needs 2 global flags namespace, address, I think it's fine passing the two instead of a whole GOptions * types.GlobalCommandOptions.

Meanwhile, for compose (sub)commands, a good start point is the getComposer function:

func getComposer(cmd *cobra.Command, client *containerd.Client) (*composer.Composer, error) {

It can be splitted into 2 sub functions:

  1. getComposeOptions(cmd *cobra.Command) composer.Options that on the cmd side and construct the composer.Options from cmd. (basically, the first part of original function).
  2. getComposer(opt composer.Options, client *containerd.Client, stdout, stderr io.Writer) (*composer.Composer, error) that on the pkg side and accept a compose option and stdout/stderr from cmd and construct the composer (the rest of original function).

@Laitr0n
Copy link
Copy Markdown
Contributor Author

Laitr0n commented Jan 4, 2023

  1. What is commonOpts types.CommonOptions?

  2. If NewClient only needs 2 global flags namespace, address, I think it's fine passing the two instead of a whole GOptions * types.GlobalCommandOptions.

  1. As you see, commonOpts types.CommonOptions come from PR [Refactor] Refactor the build subcommand flagging process #1792 which is outdated after you reviewed.
  2. SGTM

Meanwhile, for compose (sub)commands, a good start point is the getComposer function:

func getComposer(cmd *cobra.Command, client *containerd.Client) (*composer.Composer, error) {

It can be splitted into 2 sub functions:

  1. getComposeOptions(cmd *cobra.Command) composer.Options that on the cmd side and construct the composer.Options from cmd. (basically, the first part of original function).

  2. getComposer(opt composer.Options, client *containerd.Client, stdout, stderr io.Writer) (*composer.Composer, error) that on the pkg side and accept a compose option and stdout/stderr from cmd and construct the composer (the rest of original function).

SGTM, I will finish this.

@Laitr0n Laitr0n force-pushed the refactot_compose_build branch 2 times, most recently from 68d738b to 0580074 Compare January 6, 2023 02:35
Signed-off-by: Laitron <meetlq@outlook.com>
Signed-off-by: Laitron <meetlq@outlook.com>
@Laitr0n Laitr0n closed this Jan 6, 2023
@Laitr0n Laitr0n deleted the refactot_compose_build branch January 6, 2023 13:38
@Laitr0n Laitr0n restored the refactot_compose_build branch January 6, 2023 13:38
@Laitr0n Laitr0n reopened this Jan 6, 2023
@Laitr0n Laitr0n force-pushed the refactot_compose_build branch from 0580074 to e66e8bd Compare January 6, 2023 13:47
@Laitr0n Laitr0n closed this Jan 10, 2023
@Laitr0n Laitr0n deleted the refactot_compose_build branch January 10, 2023 13:46
@Laitr0n Laitr0n restored the refactot_compose_build branch January 10, 2023 14:00
@Laitr0n Laitr0n reopened this Jan 10, 2023
@Laitr0n Laitr0n closed this Jan 10, 2023
@Laitr0n Laitr0n reopened this Jan 10, 2023
@Laitr0n Laitr0n closed this Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants