Make volume ls output order #20389
Make volume ls output order #20389
Conversation
Fixes: moby#20384 Add order support for volume ls to make it easy to external users to consume. Signed-off-by: Kai Qiang Wu(Kennan) <wkqwu@cn.ibm.com>
HackToday
force-pushed
the
addvolumesupport
branch
from
9ace61d
to
60ffd6c
Compare
| // Since there is no guarantee of ordering of volumes, we just make sure the names are in the output | ||
| c.Assert(strings.Contains(out, id+"\n"), check.Equals, true) | ||
| c.Assert(strings.Contains(out, "test\n"), check.Equals, true) | ||
| assertVolList(c, out, []string{"aaa", "soo", "test"}) |
There was a problem hiding this comment.
Can we be 100% sure that no other volumes exist from other tests? Just wondering if there's a chance another volume gets into the list, making the test become flaky
There was a problem hiding this comment.
hi @thaJeztah right now, I did not find the good reason the volumes left there, it means other test not good deal with clean up. So it seems if it happen, it means other tests not good written.
Wdyt? If we can not assume others test good written. what do you think the best way to test the sort behavior ?
There was a problem hiding this comment.
I think that;
- In this case perhaps
assertVolListcan skip volumes that are not in the list ofexpectedVols? Alternatively, only check if they are all present, and appear the expected order - We should make sure that (just as with containers) all volumes are removed after a test completes (but that can be a separate PR)
There was a problem hiding this comment.
If a volume is left in the list, then there is a bug somewhere since we clean up all volumes on every test.
There was a problem hiding this comment.
So @thaJeztah @cpuguy83 seems the above "left volume" issue is not related with this PR. as all other tests follow same logic it depends on
func (s *DockerSuite) TearDownTest(c *check.C) {
unpauseAllContainers()
deleteAllContainers()
deleteAllImages()
deleteAllVolumes()
deleteAllNetworks()
}
That DockerSuite would did that, I think, we not need to call delete volume in single test
There was a problem hiding this comment.
Agreed; if the TearDown removes all volumes, individual tests should not call delete volume
cpuguy83
added
status/2-code-review
status/1-design-review
and removed
status/0-triage
status/2-code-review
labels
|
Just to have the discussion... should this also be grouped by driver? |
Not for now, I think; we don't currently group, e.g., containers per "image" used? Being able to |
|
Yes, I agree with @thaJeztah for related group behavior, we need use filter ways, instead of depend the ls output behavior |
|
ping @thaJeztah and @cpuguy83 Could you check above comments for the test ? Thanks |
|
@cpuguy83 agreed with @thaJeztah. Let's keep it simple for now and not group by driver. Adding a |
|
We should keep this consistent with the |
|
@cpuguy83 network in under review, it use same logic sort with name |
|
network is here #20383 |
|
LGTM |
1 similar comment
|
LGTM |
Fixes: #20384
Add order support for volume ls to make it easy
to external users to consume.
Signed-off-by: Kai Qiang Wu(Kennan) wkqwu@cn.ibm.com