Skip to content
This repository has been archived by the owner on Mar 21, 2022. It is now read-only.

Changes to loading and saving #494

Merged
merged 2 commits into from
Sep 20, 2016
Merged

Changes to loading and saving #494

merged 2 commits into from
Sep 20, 2016

Conversation

davidxia
Copy link
Contributor

@davidxia davidxia commented Sep 19, 2016

Fix for #154 and #464.

Load and Create

Previously, the various DockerClient.load(...) methods all wrapped the /images/create docker API endpoint, not the /images/load endpoint as the method name implies. In fact, no methods in DockerClient wrapped /images/load.

This PR:

  • Deprecates all existing DockerClient.load(...) methods;
  • Creates two DockerClient.create(...) methods, the implementations of which perform the functions that DefaultDockerClient.load(...) used to perform;
  • Creates a new DockerClient.load(ImageStream) method, the implementation of which calls /images/load.

Save

  • There was no reason to use the AuthConfig when calling /images/{name}/get. The method DockerClient.save(String, AuthConfig) has been deprecated.
  • The other save method, DockerClient.save(String) has been changed to DockerClient.save(String...). This is to make use of the two related APIs /images/{name}/get and /images/get?names=name1&names=name2....

@mattnworb
Copy link
Member

What is the purpose of using /images/create instead of /images/load? In other words what is motivating that change?

@codecov-io
Copy link

codecov-io commented Sep 19, 2016

Current coverage is 50.23% (diff: 15.00%)

Merging #494 into master will decrease coverage by 0.31%

@@             master       #494   diff @@
==========================================
  Files            83         83          
  Lines          3219       3235    +16   
  Methods           0          0          
  Messages          0          0          
  Branches        456        460     +4   
==========================================
- Hits           1627       1625     -2   
- Misses         1443       1459    +16   
- Partials        149        151     +2   

Powered by Codecov. Last update 3c985eb...0ac3f68

@davidxia
Copy link
Contributor Author

@mattnworb The client will support both now. I think @johnflavin was just trying to make the method names match the API endpoint names. Does that make sense?

@mattnworb
Copy link
Member

I think I misunderstood why the existing DockerClient.load(..) methods were being marked as deprecated. Is it simply that some of the existing methods are confusing in that they do not match the names of the Docker Remote API methods that they call, so they are deprecated to point people to use the better-named methods?

Copy link
Member

@mattnworb mattnworb left a comment

Choose a reason for hiding this comment

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

anyway, the change LGTM. Thanks again @johnflavin for the contribution! I think this definitely reduces confusion around the method names.

@mattnworb
Copy link
Member

also the TravisCI errors look completely unrelated.

@davidxia
Copy link
Contributor Author

@mattnworb

Is it simply that some of the existing methods are confusing in that they do not match the names of the Docker Remote API methods that they call, so they are deprecated to point people to use the better-named methods?

Yup

@davidxia
Copy link
Contributor Author

@mattnworb Any idea why travis fails for those two docker versions?

@mattnworb
Copy link
Member

@davidxia I think some assumption we made in our script that installs a specific docker version in travis's environment must have changed or been broken by them

@davidxia davidxia merged commit d00d53c into master Sep 20, 2016
@davidxia davidxia deleted the dxia/load branch September 20, 2016 20:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants