Add scope filter in GET /networks/(id or name)
#33630
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/cc @dnephin |
dnephin
reviewed
Jun 11, 2017
| @@ -112,15 +113,15 @@ func (n *networkRouter) getNetwork(ctx context.Context, w http.ResponseWriter, r | |||
|
|
|||
| nw := n.backend.GetNetworks() | |||
| for _, network := range nw { | |||
| if network.ID() == term { | |||
| if network.ID() == term && (len(scope) == 0 || network.Info().Scope() == scope) { | |||
There was a problem hiding this comment.
A isMatchingScope(network, scope) would be great here so it can be used in all 6 places.
not: I think comparing string to "" makes the intent more obvious.
There was a problem hiding this comment.
Thanks @dnephin. The PR has been updated.
client/interface.go
Outdated
| NetworkInspect(ctx context.Context, networkID string, verbose bool) (types.NetworkResource, error) | ||
| NetworkInspectWithRaw(ctx context.Context, networkID string, verbose bool) (types.NetworkResource, []byte, error) | ||
| NetworkInspect(ctx context.Context, networkID string, scope string, verbose bool) (types.NetworkResource, error) | ||
| NetworkInspectWithRaw(ctx context.Context, networkID string, scope string, verbose bool) (types.NetworkResource, []byte, error) |
There was a problem hiding this comment.
Now that there is two flags,
type NetworkInspectOptions struct {
Scope string
Verbose bool
}
would be great, instead of adding more args.
client/network_inspect_test.go
Outdated
| @@ -51,6 +51,13 @@ func TestNetworkInspect(t *testing.T) { | |||
| content []byte | |||
| err error | |||
| ) | |||
| if strings.HasPrefix(req.URL.RawQuery, "scope=global") { | |||
There was a problem hiding this comment.
This and the check below should probably use Contains now
| d := s.AddDaemon(c, true, true) | ||
|
|
||
| out, err := d.Cmd("network", "create", "-d", "overlay", "foo") | ||
| c.Assert(err, checker.IsNil, check.Commentf(out)) |
There was a problem hiding this comment.
Not sure about this. I think we probably want to create using an API call as well. Maybe it's fine for now.
Using the docker client should make it just as easy to create the network.
There was a problem hiding this comment.
Updated. Now API call is used to create the network.
client/network_inspect_test.go
Outdated
| @@ -93,4 +100,9 @@ func TestNetworkInspect(t *testing.T) { | |||
| if !ok { | |||
| t.Fatalf("expected service `web` missing in the verbose output") | |||
| } | |||
|
|
|||
| _, err = client.NetworkInspect(context.Background(), "network_id", "global", false) | |||
| if !IsErrNetworkNotFound(err) { | |||
This fix tries to add a `scope` in the query of `/networks/<id>` (`NetworkInspect`) so that in case of duplicate network names, it is possible to locate the network ID based on the network scope (`local`, 'swarm', or `global`). Multiple networks might exist in different scopes, which is a legitimate case. For example, a network name `foo` might exists locally and in swarm network. However, before this PR it was not possible to query a network name `foo` in a specific scope like swarm. This fix fixes the issue by allowing a `scope` query in `/networks/<id>`. Additional test cases have been added to unit tests and integration tests. This fix is related to docker/cli#167, moby#30897, moby#33561, moby#30242 This fix fixes docker/cli#167 Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
yongtang
force-pushed
the
167-cli-network-inspect-scope
branch
from
8255bca
to
158b2a1
Compare
|
Thanks @dnephin for the review. The PR has been updated. Please take a look. |
dnephin
approved these changes
Jun 12, 2017
| var n types.NetworkCreateResponse | ||
| networkCreateRequest.NetworkCreate.Driver = "overlay" | ||
|
|
||
| status, out, err := d.SockRequest("POST", "/networks/create", networkCreateRequest) |
There was a problem hiding this comment.
You can also use the docker client/ here, which you can get from `integration-cli/request.NewClient(), but this is ok as well I guess.
|
LGTM |
andrewhsu
pushed a commit
to docker-archive/docker-ce
that referenced
this pull request
Jun 24, 2017
This fix updates docker/docker to 4310f7d This fix is related to moby/moby#33630 and docker/cli#167 Signed-off-by: Yong Tang <yong.tang.github@outlook.com> Upstream-commit: 8c2f81892beb7634a97efeec053f7052c36b4039 Component: cli
andrewhsu
pushed a commit
to docker-archive/docker-ce
that referenced
this pull request
Jun 24, 2017
This fix use `scope=swarm` for service related network inspect. The purpose is that, in case multiple networks with the same name exist in different scopes, it is still possible to obtain the network for services. This fix is related to moby/moby#33630 and docker/cli#167 Signed-off-by: Yong Tang <yong.tang.github@outlook.com> Upstream-commit: 657457ee2cc1b4ced804674ad1b3bfe19b849f31 Component: cli
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
- What I did
This fix tries to add a
scopein the query ofGET /networks/(id or name)(NetworkInspect) so that in case of duplicate network names, it is possible to locate the network ID based on the network scope (local, 'swarm', orglobal).Multiple networks might exist in different scopes, which is a legitimate case. For example, a network name
foomight exists locally and in swarm network.However, before this PR it was not possible to query a network name
fooin a specific scope like swarm.- How I did it
This fix fixes the issue by allowing a
scopequery inGET /networks/(id or name).- How to verify it
Additional test cases have been added to unit tests and integration tests.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)
This fix is related to docker/cli#167, #30897, #33561, #30242
This fix fixes docker/cli#167
Signed-off-by: Yong Tang yong.tang.github@outlook.com