Not sure whether this part is needed.
danglingOnly is bool, which means it can be true or false.

Makes sense, will remove that 👍

NOTE: This is a suggestion.
I would prefer get rid of the switch block and do the following:

for _, nw := range nws {
    if !runconfig.IsPreDefinedNetwork(nw.Name) && len(nw.Containers) == 0 && danglingOnly {
        retNws = append(retNws, nw)
    }
}

I think it'll work.

But how will this work when dangling=false. In that case we'd have to append non-dangling networks to retNws. Which is done only when, either, the network has been attached to more than a container or is a pre-defined network.

Yeah, I knew I missed something.
What about setting a filter function instead of using a condition.

filterFunc := func(nw types.NetworkResource) bool {
    if danglingOnly {
        return !runconfig.IsPreDefinedNetwork(nw.Name) && len(nw.Containers) == 0
    }
    return runconfig.IsPreDefinedNetwork(nw.Name) || len(nw.Containers) > 0
}
...
for _, nw := range nws {
    if filterFunc(nw) {
        retNws = append(retNws, nw)
    }
}

Even though I believe it's possible to put those conditions in one line, I am afraid it won't be readable.

That works too, but I don't see why we need to do this?

I mean the code currently tries to be consistent with the existing function filterNetworksByType, and staying consistent would mean better readability. Not that I'm against your suggestion, I just don't see any advantage in doing so.

i like @ripcurld0 idea 😊

@vdemeester I'm ok with changing to that, but that wouldn't be consistent with the existing code, that's all.

This does not work for swarm networks, because nw.Containers does not contain containers running on other nodes

I see, any suggestions how to go about this?

My opinion is to add DryRun to RemoveNetworkRequest.
https://github.com/docker/swarmkit/blob/450df1ca02727e79b4c58e8448173150f3c56c25/api/control.proto#L289-L292

cc @aluzzardi @aaronlehmann

That would mean dropping this commit and re-working, i.e. adding the DryRun option to RemoveNetworkRequest and perhaps then have a this filter just call that?

yes, unfortunately..

IIUC, ListTasksRequest does not work for docker run containers.
i.e.

$ docker network create --driver overlay --attachable foo
$ docker run --network foo ...

I would expect it to work for docker run containers attached to networks. Those are represented by tasks, and I don't know of anything that filters them out in swarmkit (maybe there is something filtering them in the engine?).

So basically this PR would be dependent on #24186, right?

Probably yes. But it would be OK if ListTasksRequest work.

Now that #31710 is accepted, I'll finish this soon 👍

I believe these fields are provided by the gossip layer and are eventually consistent. Is it a problem if the dangling filter may provide results that are slightly out of date?

I don't think I know enough to answer that. I'll look around.

cc @mavenugo @aboch

I don't think it should be a problem as long as the consumer knows this is potentially racey.
Trying to rm a network from this list wouldn't really pose a problem except for potentially racey behavior from the state propagation, but this problem would exist in any case.

add "0" and "1" ? Print->Prints>
WDYT

will do s/Print/Prints.

About '0' and '1', I'm a little confused.

~ docker image ls --filter dangling=0
Error response from daemon: Invalid filter 'dangling=[0]'

~ docker volume ls --filter dangling=0
DRIVER              VOLUME NAME

So I'm on the fence with that

Yeah, I am just referenced to https://github.com/docker/docker/blob/master/api/swagger.yaml#L5089-L5091 for image prune dangling:

- `dangling=<boolean>` When set to `true` (or `1`), prune only
               unused *and* untagged images. When set to `false`
               (or `0`), all unused images are pruned.

Wonder if we should be explicit here, and add danglingOnly = false. Perhaps because I'm used to "fall-through" being the default in other languages, but I had to look twice to interpret this correctly 😊

It's already false?

Yeah, I know, it's probably just me, reading;

switch {
  case foo:
    dofoo()
  case bar:
    dobar()
  case baz:
  default:
    dosomethingelse()
}

I always tend to first read that as baz: falls through to default: (so error in this case)

Actually; should this use filter.ExactMatch() here?

I'm not sure why one would be better than the other. This filters package is pretty confusing.

Include() is deprecated; can you change to Contains()?

Is it a concern that when filtering for "dangling", the returned results will be different than without? (i.e. it having "detailed" information)?

Looking at buildDetailedNetworkResources(), and how we use that information during filtering (we're only interested in knowing if containers or services are attached);

Wondering if we should have a simplified version, or at least reset the Containers and Services information afterwards, so that the information that's returned for the network is consistent.

Sorry to do this, but do you think we can alter the test to actually test for which networks should be returned? Can you also add a network that is not a built-in but is not dangling?

@cpuguy83 Haha, I'll get to it soon 👍

@cpuguy83 Done! 👍