Skip to content

cmd/containerd: split package for cli.App#2131

Merged
estesp merged 1 commit intocontainerd:masterfrom
AkihiroSuda:split-pkg-cmd-containerd-2
Feb 14, 2018
Merged

cmd/containerd: split package for cli.App#2131
estesp merged 1 commit intocontainerd:masterfrom
AkihiroSuda:split-pkg-cmd-containerd-2

Conversation

@AkihiroSuda
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda commented Feb 14, 2018

Signed-off-by: Akihiro Suda [email protected]

#1615 (comment)

I think we need to split this PR up into separate steps. There is too much going on and its trying to solve too many things from the discussion but not delivering in the code.

The two things we want to solve are:

  1. Better plugin registration/imports
  2. Plugins adding CLI functionality
    From the code I see in this PR, I think it is safe to solve (2) first. We can export the App and provide a way for plugins to register cli.Command that are later assembled in the main().

Then once that is done, we can start on the default registration/imports. These are two problems but we are not really solving any of the current state of discussion and code of this PR.

This PR addresses the issue (2) separately.

@AkihiroSuda AkihiroSuda mentioned this pull request Feb 14, 2018
Closed
@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 14, 2018

Codecov Report

Merging #2131 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2131   +/-   ##
=======================================
  Coverage   45.53%   45.53%           
=======================================
  Files          80       80           
  Lines        8726     8726           
=======================================
  Hits         3973     3973           
  Misses       4107     4107           
  Partials      646      646
Flag Coverage Δ
#linux 50.24% <ø> (ø) ⬆️
#windows 41.09% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af593cf...d7280ce. Read the comment docs.

@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Feb 14, 2018

LGTM

estesp
estesp approved these changes Feb 14, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@estesp estesp merged commit 002caba into containerd:master Feb 14, 2018
@AkihiroSuda AkihiroSuda mentioned this pull request Feb 15, 2018
Closed
@Random-Liu
Copy link
Copy Markdown
Member

Random-Liu commented Feb 21, 2018

Just noticed that this is merged! Will use this in cri-containerd for CI testing!

@Random-Liu Random-Liu mentioned this pull request Feb 21, 2018
Merged
This was referenced Feb 28, 2018
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@estesp estesp estesp approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@AkihiroSuda @codecov-io @crosbymichael @Random-Liu @estesp