The Wayback Machine - https://web.archive.org/web/20251014211538/https://github.com/moby/moby/pull/36688
Skip to content

Conversation

cpuguy83
Copy link
Member

This cleans up some of the package API's used for interacting with volumes, and simplifies management.

This is built on top of #36637.

I've done a lot of manual testing on this, but would like to add some more tests especially around the filtering logic, which is also now consolidated to the volume service.

Copy link
Member

Choose a reason for hiding this comment

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

This seems to be a very different abstraction from what we are calling services currently (ex: ImageService, and RegistryService) which implement higher level logic.

Most of this interface is implemented by the VolumeStore, right?

I would think a VolumeService would be a type that implements api/server/router/volume.Backend, as well as whatever is used by other services. That way we remove the need for a monolithic daemon package.

I like the changes in this PR, but it seems that we're losing the ability to create a VolumeService to replace the daemon methods for volumes.

Could this continue to be a VolumeStore, and we split out a service later?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could call it that still... but that (replace daemon methods) is indeed the intent, it's just yet more change to add in this large PR, of course can add a commit to make that happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

Made this change.

@cpuguy83 cpuguy83 changed the title [WIP] Extract volume interaction to a volumes service Extract volume interaction to a volumes service Apr 18, 2018
@cpuguy83 cpuguy83 force-pushed the volumes_service branch 2 times, most recently from 983da3c to eaac2a9 Compare April 19, 2018 01:04
@codecov
Copy link

codecov bot commented Apr 19, 2018

Codecov Report

Merging #36688 into master will decrease coverage by 0.12%.
The diff coverage is 52.7%.

@@            Coverage Diff             @@
##           master   #36688      +/-   ##
==========================================
- Coverage   35.05%   34.93%   -0.13%     
==========================================
  Files         615      609       -6     
  Lines       45828    44973     -855     
==========================================
- Hits        16065    15711     -354     
+ Misses      27652    27136     -516     
- Partials     2111     2126      +15

@cpuguy83 cpuguy83 force-pushed the volumes_service branch 9 times, most recently from 511108e to 81b74d8 Compare April 20, 2018 20:32
@cpuguy83 cpuguy83 changed the title Extract volume interaction to a volumes service [WIP] Extract volume interaction to a volumes service Apr 23, 2018
@cpuguy83 cpuguy83 force-pushed the volumes_service branch 3 times, most recently from f1c8283 to e14f9b3 Compare April 24, 2018 18:19
@cpuguy83
Copy link
Member Author

Updated and all green.

This cleans up some of the package API's used for interacting with
volumes, and simplifies management.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
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

@thaJeztah
Copy link
Member

ping @anusha-ragunathan PTAL

@anusha-ragunathan
Copy link
Contributor

LGTM

@thaJeztah thaJeztah merged commit 5037c5a into moby:master Jun 5, 2018
@cpuguy83 cpuguy83 deleted the volumes_service branch June 5, 2018 03:20
@zq-david-wang
Copy link
Contributor

@cpuguy83 unit tests TestServiceCreate/TestServiceGet may fail
https://jenkins.dockerproject.org/job/Docker-PRs-WoW-RS1/21046/console

13:35:39 --- FAIL: TestServiceCreate (0.05s)
13:35:39 service_test.go:38: assertion failed:
13:35:39 {*types.Volume}.CreatedAt:
13:35:39 -: "2018-06-05T13:35:12Z"
13:35:39 +: "2018-06-05T13:35:13Z"
13:35:39
13:35:39 --- FAIL: TestServiceGet (0.08s)
13:35:39 service_test.go:144: assertion failed:
13:35:39 {*types.Volume}.CreatedAt:
13:35:39 -: "2018-06-05T13:35:12Z"
13:35:39 +: "2018-06-05T13:35:13Z"
13:35:39

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.

8 participants