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.

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

Export pull as a public function #1026

Merged
merged 1 commit into from
May 2, 2018
Merged

Export pull as a public function #1026

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions cli/command/image/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import (
"golang.org/x/net/context"
)

type pullOptions struct {
// PullOptions defines what and how to pull
type PullOptions struct {
remote string
all bool
platform string
Expand All @@ -22,15 +23,15 @@ type pullOptions struct {

// NewPullCommand creates a new `docker pull` command
func NewPullCommand(dockerCli command.Cli) *cobra.Command {
var opts pullOptions
var opts PullOptions

cmd := &cobra.Command{
Use: "pull [OPTIONS] NAME[:TAG|@DIGEST]",
Short: "Pull an image or a repository from a registry",
Args: cli.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
opts.remote = args[0]
return runPull(dockerCli, opts)
return RunPull(dockerCli, opts)
},
}

Expand All @@ -44,7 +45,8 @@ func NewPullCommand(dockerCli command.Cli) *cobra.Command {
return cmd
}

func runPull(cli command.Cli, opts pullOptions) error {
// RunPull performs a pull against the engine based on the specified options
func RunPull(cli command.Cli, opts PullOptions) error {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit unsure if this is the right functionality to export, i.e.; what exact functionality are you looking for? Just pulling a specific image, or also Including the option to pull --all-tags? Having an error when attempting to pull a plugin? If new options are added to docker pull should they be automatically be added to the other parts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm certainly open to suggestions on refinement...

The use-case I was looking at was a higher level construct that uses helper images to accomplish a task in the CLI. Conceptually equivalent to:

docker pull someimagethatmightbeprivate:tag
docker run -v /var/lib/docker.sock:/var/lib/docker.sock someimagethatmightbeprivate:tag dowork

The main thing I was trying to DRY out was all the logic around dealing with the credential cache for logins. My thinking was we might need the knobs and dials that docker pull exposes so exposing the high level CLI function/options could be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you think that's too high-level/abstract given the current requirements, perhaps a different approach is to create a new helper in the trust package that takes a repo, and coughs out an authConfig struct suitable to pass into the standard engine-api Pull routine after base64 encoding it?

Copy link
Member

Choose a reason for hiding this comment

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

Got it; yes, we could probably have something that can be shared (just wanted to be cautious to not introduce dependencies between commands if not needed; the PullOptions and error-handling looked very specific to docker pull)

@dnephin @vdemeester wdyt? suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems fine to me as long as it can be used as-is.

If we need to introduce any changes for the new caller I think we should refactor to expose a different interface to accommodate the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we need to introduce any changes for the new caller I think we should refactor to expose a different interface to accommodate the changes.

Agreed.

At this point I don't think we need to make any changes.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like we're ok to (at least for now) export this function 👍

distributionRef, err := reference.ParseNormalizedNamed(opts.remote)
switch {
case err != nil:
Expand Down