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

Export pull as a public function #1026

Merged
merged 1 commit into from
May 2, 2018
Merged

Conversation

dhiltgen
Copy link
Contributor

It will be helpful to expose the pull implementation which supports
pulling private images for other CLI commands that rely on helper images.

Signed-off-by: Daniel Hiltgen daniel.hiltgen@docker.com

It will be helpful to expose the pull implementation which supports
pulling private images for other CLI commands that rely on helper images.

Signed-off-by: Daniel Hiltgen <daniel.hiltgen@docker.com>
@@ -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 👍

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@dnephin ptal

@thaJeztah thaJeztah merged commit 7844307 into docker:master May 2, 2018
@GordonTheTurtle GordonTheTurtle added this to the 18.05.0 milestone May 2, 2018
@thaJeztah thaJeztah modified the milestones: 18.05.0, 18.06.0 May 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants