8000 cmd/containerd: split package for cli.App by AkihiroSuda · Pull Request #2131 · containerd/containerd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 14, 2018
Merged

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

merged 1 commit into from
Feb 14, 2018

Conversation

AkihiroSuda
Copy link
Member

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

#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
cmd/containerd: split package for cli.App
d7280ce 
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@AkihiroSuda AkihiroSuda mentioned this pull request Feb 14, 2018
Closed
@codecov-io
Copy link

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
Member

LGTM

estesp
estesp approved these changes Feb 14, 2018
View reviewed changes
Copy link
Member
@estesp estesp left a comment

Choose a 8000 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
Member

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
0