use apiClient for various things #1982
Conversation
|
Adding reference to #1879, this is part of implementing that proposal. |
|
Yup, I should have marked that. I wanted to give a shot at starting. I might combine some more stuff into this as individual commits. I would like to get some basic eyes-on to start. |
|
@MHBauer thanks for helping out with moving to the new client. I'm thinking that it would be a good idea to add some unit tests so that we can monitor these changes over time to ensure stability. |
|
I'll have to think about how best to do that. I was mainly making sure the integration tests still ran, as I assume those are using the actual clients, vs what would probably be a mock here. |
MHBauer
changed the title
|
@MHBauer that's correct, but unit tests are valuable to catch certain issues integration tests won't. Your changes seem to be isolated enough so it may not be a problem, but in general it would be nice to have unit tests in all packages that use the new client. |
| @@ -471,17 +473,17 @@ func (e *Engine) updateSpecs() error { | |||
| } | |||
|
|
|||
| // RemoveImage deletes an image from the engine. | |||
| func (e *Engine) RemoveImage(image *Image, name string, force bool) ([]*dockerclient.ImageDelete, error) { | |||
| array, err := e.client.RemoveImage(name, force) | |||
| func (e *Engine) RemoveImage(image *Image, name string, force bool) ([]types.ImageDelete, error) { | |||
There was a problem hiding this comment.
What is the purpose if the image *Image parameter? It wasn't used before, and it's not used after.
There was a problem hiding this comment.
Good point, seems like a redundant argument. You can consider removing it. I'm surprised the compiler didn't complain about it before.
|
@nishanttotla test added for |
|
I think this is enough for one PR. @docker/core-swarm-maintainers for review, please. |
|
rebased. I think some lines got moved around incorrectly. I am checking that. The end result does build, and the context that the commits provide to each other is all self-contained within this PR. |
|
I highly doubt it. I don't want to keep punching the build machine til it works. Integration-tests work locally. Can you reproduce the failure? I'll run them again here. |
|
@MHBauer if they work locally then it's fine, I just wanted to be sure :) |
|
@MHBauer what's the status of this PR? I'm slightly surprised that there are no merge conflicts, so can you make sure that everything's alright? :) We'd like to merge this for 1.2. |
|
@MHBauer please rebase :) |
|
LGTM |
|
Well, at least it's the same test failing. Rebased. Investigating integration test |
|
reproduces locally as well. |
|
hmn, it's failing on master as well. Now I'm confused. |
|
@MHBauer it's unrelated. @nishanttotla let's merge |
| apiClient.On("NetworkList", mock.Anything, | ||
| mock.AnythingOfType("NetworkListOptions"), | ||
| ).Return([]types.NetworkResource{}, nil) | ||
| apiClient.On("VolumeList", mock.Anything, mock.Anything).Return(types.VolumesListResponse{}, nil) |
There was a problem hiding this comment.
@MHBauer could you do something like mock.AnythingOfType("VolumeListOptions") to be consistent with the above?
There was a problem hiding this comment.
No. VolumeList takes a generic filters.Args. NetworksList has a specific struct that it takes.
I can try mock.AnythingOfType("Args") but that might be less helpful than just being Anything. What do you think?
There was a problem hiding this comment.
I'd rather that we be as specific as possible. Especially since all types are changing, and just in case any stray arguments get passed it'd be nice to know (even though for this particular case I don't see that happening). It also helps with debugging in the future.
|
@MHBauer it looks good overall, just some minor fixes :) |
- remove unused argument - unit test of new client Signed-off-by: Morgan Bauer <mbauer@us.ibm.com>
Signed-off-by: Morgan Bauer <mbauer@us.ibm.com>
Signed-off-by: Morgan Bauer <mbauer@us.ibm.com>
Signed-off-by: Morgan Bauer <mbauer@us.ibm.com>
- create volume - list volumes Signed-off-by: Morgan Bauer <mbauer@us.ibm.com>
|
LGTM |
|
Same two flaky tests, nothing more, so I'll merge it. |
|
LGTM. Test failures unrelated, as confirmed by @abronan. |
|
Or you'll merge it. That's cool too. |
|
@MHBauer 🎉 |
use apiClient for various things
engine-api has has the prune/noprune option. The matching behavior is to prune. Not sure if we want or need to make that option available with the pass through.
Not sure if the array of pointers vs array of objects needs to be looked at or if it's fine how it is.