Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

Move the inspect command to "image inspect" #699

Merged
merged 2 commits into from
Oct 18, 2019
Merged

Conversation

rumpl
Copy link
Member

@rumpl rumpl commented Oct 17, 2019

- What I did

The inspect command needed a bunch of code that was only present in the
"internal/commands" package. The patch is big but it's only moving code
around:

  • cnab.go was split into multiple files:
    • credentials.go -> all things credentials
    • driver/docker.go -> mainly for the driver.Prepare that all the commands use
    • registry.go -> has the method to get insecure registries from engine

- A picture of a cute animal (not mandatory but encouraged)
image

@codecov
Copy link

codecov bot commented Oct 17, 2019

Codecov Report

Merging #699 into master will increase coverage by 0.05%.
The diff coverage is 74.1%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #699      +/-   ##
==========================================
+ Coverage   71.28%   71.34%   +0.05%     
==========================================
  Files          56       59       +3     
  Lines        2929     2935       +6     
==========================================
+ Hits         2088     2094       +6     
  Misses        567      567              
  Partials      274      274
Impacted Files Coverage Δ
internal/commands/run.go 65.71% <100%> (ø) ⬆️
internal/commands/list.go 84.12% <100%> (ø) ⬆️
internal/commands/pull.go 69.23% <100%> (ø) ⬆️
internal/commands/image/inspect.go 73.33% <100%> (ø)
internal/commands/image/tag.go 87.87% <100%> (-1.32%) ⬇️
internal/commands/image/command.go 100% <100%> (ø) ⬆️
internal/commands/image/rm.go 61.29% <100%> (ø) ⬆️
internal/commands/push.go 45.22% <100%> (ø) ⬆️
internal/commands/remove.go 52% <100%> (ø) ⬆️
internal/dockerdesktop.go 46.75% <50%> (ø)
... and 13 more

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 f611ee7...2cdb76e. Read the comment docs.

@rumpl rumpl force-pushed the feat-refactor branch 3 times, most recently from 736983d to 3891788 Compare October 17, 2019 15:43
@@ -16,6 +18,15 @@ type inspectOptions struct {
pretty bool
}

func muteDockerCli(dockerCli command.Cli) func() {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we make it public instead of duplicating it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had a private conversation with @silvin-lubecki and:

  • no idea where to put it
  • it will go away soon because we started working on cnab-go to make it take an stdout so that we don't have to mute the cli any more

This is like a reminder for us that we really need to continue working on cnab-go

Copy link
Member

Choose a reason for hiding this comment

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

👍

The inspect command needed a bunch of code that was only present in the
"internal/commands" package. The patch is big but it's only moving code
around:

* cnab.go was split into multiple files:
  * credentials.go -> all things credentials
  * driver/docker.go -> mainly for the driver.Prepare that all the
commands use
  * registry.go -> has the method to get insecure registries from engine

Signed-off-by: Djordje Lukic <[email protected]>
@rumpl rumpl force-pushed the feat-refactor branch 2 times, most recently from a15220a to a91eb40 Compare October 18, 2019 09:57
Cobra makes sure that it's thee

Signed-off-by: Djordje Lukic <[email protected]>
Copy link
Contributor

@jcsirot jcsirot left a comment

Choose a reason for hiding this comment

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

LGTM

@rumpl rumpl merged commit ab69992 into docker:master Oct 18, 2019
@rumpl rumpl deleted the feat-refactor branch October 18, 2019 12:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants