Skip to content
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.

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

feature: support multiple inspection #989

Merged
merged 1 commit into from
Mar 29, 2018

Conversation

faycheng
Copy link
Contributor

@faycheng faycheng commented Mar 28, 2018

Signed-off-by: 程飞 fay.cheng.cn@gmail.com

Ⅰ. Describe what this PR did

Support multiple inspection inspired by the issue #922

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

➜ pouch image inspect -f '{{.ID}}' 4493f9e1fd39 786f81bbee21
sha256:4493f9e1fd3990cc8e4ea30e8b9583a90b6596de488cd95284bf0df5823baf3e
sha256:786f81bbee21734ff4bd5b7e30edea495200a28e10c2999b50f553a816391284

Ⅴ. Special notes for reviews

@allencloud
Copy link
Collaborator

I am afraid that travisCI fails to run this PR, since there is one ci test missing.
Could you help to force push your branch with a minor change? @faycheng

Also cc @HusterWan @Letty5411 to see is there anything we could do to guarantee the CI testing.

@faycheng
Copy link
Contributor Author

@allencloud Done

@@ -8,6 +8,7 @@ import (

"github.com/alibaba/pouch/pkg/utils/templates"

"fmt"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move this built package fmt to line L6 of this file and gofmt this file, since we have made a rule of

When importing packages, to improve readabilities, we should import package by sequence: system packages, project's own packages and third-party packages. And we should keep a blank line among these three kinds of packages;

In https://github.com/alibaba/pouch/blob/master/docs/contributions/code_styles.md#additional-style-rules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

omg, I will solve it.
Thanks for your reminder.

RunE: func(cmd *cobra.Command, args []string) error {
return i.runInspect(args)
Args: cobra.MinimumNArgs(1),
Run: func(cmd *cobra.Command, args []string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not think that change RunE into Run is correct. Here is my reason: we need to know whether this command works or not via RunE, since RunE would return an error or nil.

With Run, we cannot tell whether the command runs into error, this is a confusing situation for users. Users cannot use echo $? to judge if it runs well, since the exit code is always 0.

WDYT? @faycheng

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right.I will rollback to RunE.

@faycheng faycheng changed the title feature: support multiple inspection [WIP]feature: support multiple inspection Mar 28, 2018
@faycheng faycheng force-pushed the multi-inspect branch 4 times, most recently from 7bc56ff to 46a4ff2 Compare March 28, 2018 16:52
@allencloud allencloud dismissed their stale review March 28, 2018 17:01

already updated change request, dismissing

@allencloud
Copy link
Collaborator

I am afraid that CI would fail since I think there is a bug in code base. PR #992 tries to fix this. You can ignore the failed cases which are described in #991 .

@faycheng
Copy link
Contributor Author

Got it.
Thanks again.Good night.

@allencloud
Copy link
Collaborator

#992 has been merged. Please rebase your branch to the latest upstream/master and push it back again. Thanks. @faycheng

Signed-off-by: 程飞 <fay.cheng.cn@gmail.com>
@codecov-io
Copy link

codecov-io commented Mar 29, 2018

Codecov Report

Merging #989 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #989      +/-   ##
==========================================
- Coverage   13.73%   13.72%   -0.02%     
==========================================
  Files         124      124              
  Lines        8358     8367       +9     
==========================================
  Hits         1148     1148              
- Misses       7111     7120       +9     
  Partials       99       99
Impacted Files Coverage Δ
cli/volume.go 9.43% <0%> (ø) ⬆️
cli/image_inspect.go 0% <0%> (ø) ⬆️
cli/inspect/inspector.go 58.46% <0%> (-9.4%) ⬇️
cli/inspect.go 0% <0%> (ø) ⬆️
cli/network.go 0% <0%> (ø) ⬆️

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 07d7840...3fbfc80. Read the comment docs.

@faycheng
Copy link
Contributor Author

@allencloud Done.
And are there any suggestions?Thanks.

@faycheng faycheng changed the title [WIP]feature: support multiple inspection feature: support multiple inspection Mar 29, 2018
RunE: func(cmd *cobra.Command, args []string) error {
return i.runInspect(args)
return inspect.MultiInspect(args, i.runInspect)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should call MultiInspect in runInspect, because we may have some context to deal with in the future, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the skill of dealing with context is necessary.and i will try to solve it.thanks.

@allencloud
Copy link
Collaborator

@faycheng Thanks for your work. We are planning that merge this PR and leave two others for you to finish:

  • Add integration test for pouch volume inspect, pouch image inspect, pouch network inspect like pouch inspect container1 container2;
  • Add context for this command

How about that?

@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Mar 29, 2018
@allencloud allencloud merged commit 98a9065 into AliyunContainerService:master Mar 29, 2018
@faycheng
Copy link
Contributor Author

@allencloud Sorry for the late reply, because i stays very busy with my work on company.
And, actually i don't finish this pr, create a new pr for integration test and ctx, or revert this pr, which is better?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature LGTM one maintainer or community participant agrees to merge the pull reuqest. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants