Update to Windows network options#801
Conversation
|
Doesn't the |
|
Yep. Updating that now. |
|
Updated |
schema/config-windows.json
Outdated
| } | ||
| "allowUnqualifiedDNSQuery": { | ||
| "id": "https://opencontainers.org/schema/bundle/windows/network/allowUnqualifiedDNSQuery", | ||
| "$ref": "defs.json#/definitions/bool" |
There was a problem hiding this comment.
Travis doesn't like bool. I think you want "type": "boolean".
There was a problem hiding this comment.
Ah, yep. Fixed it for string, but missed bool. Pushing a fix
|
|
||
| * **`egressBandwidth`** *(uint64, OPTIONAL)* - specified the maximum egress bandwidth in bytes per second for the container. | ||
| * **`endpointList`** (array of strings, OPTIONAL) list of HNS endpoints that the container should connect to. | ||
| * **`allowUnqualifiedDNSQuery`** *(bool, OPTIONAL)* - specifies if unqualified DNS name resolution is allowed. |
There was a problem hiding this comment.
This has inconsistent use of * and the trailing hyphen. The rest of master isn't very consistent either, but config.md seems to prefer:
**`{name}`** ({type}, OPTIONAL) {description}
(i.e. no * around the type/optional parens and no hyphen afterwards). It doesn't really matter which way you go, but it's probably best to avoid switching patterns in mid-list ;).
There was a problem hiding this comment.
I'm going to match the rest of this file for now (* around type/optional, hyphen after). The whole file can get converged at once in another PR that way if desired.
config-windows.md
Outdated
| The following parameters can be specified: | ||
|
|
||
| * **`egressBandwidth`** *(uint64, OPTIONAL)* - specified the maximum egress bandwidth in bytes per second for the container. | ||
| * **`endpointList`** (array of strings, OPTIONAL) list of HNS endpoints that the container should connect to. |
There was a problem hiding this comment.
If not, could link to what HNS means in official documentation.
There was a problem hiding this comment.
HNS is Host Network Service on Windows (manages all container networking). I can add clarification.
|
@wking - With the debate about whether Windows gets in for 1.0, the deprecation part of this PR is probably important for v1.0. I'm not sure if it makes sense to split this into two PRs - one for the deprecation, and one for the addition. Thoughts? |
|
On Mon, May 15, 2017 at 04:43:59PM -0700, John Howard wrote:
I'm not sure if it makes sense to split this into two PRs - one for
the deprecation, and one for the addition. Thoughts?
I'm not a stakeholder or maintainer, so it's not my call. If the
maintainer plan is to “preview” Windows in 1.0 and somehow exempt it
from SemVer (3 in [1]) or to just land changes and hope (1 in [1]), it
probably doesn't matter either way. If the goal is to keep whatever
bits of cooked Windows are already in the spec but remove the rest
(2-ish in [1]), then splitting this into two PRs makes sense. But I'm
not clear on which route the maintainers or Windows stakeholders are
leaning towards at the moment.
[1]: #817 (comment)
|
|
Changes LGTM (not a maintainer). Unfortunately needs a rebase. And may require subsequent rebases depending on the order in which the Windows PRs are merged |
|
Rebased |
|
Sorry, needs one more rebase..... |
Signed-off-by: Darren Stahl <darst@microsoft.com>
|
Rebased again |
|
LGTM |
1 similar comment
|
LGTM |
Catching 6b2fcaf (Update to Windows network options, 2017-05-10, opencontainers#801) up with the anchoring policy from style.md. Signed-off-by: W. Trevor King <wking@tremily.us>
This moves the Windows network runtime options into the spec.
NetworkResourceSettings was never used in the runtime spec, and implemented in HNS and libnetwork instead of at runtime on Windows.
/cc @RobDolinMS
Signed-off-by: Darren Stahl darst@microsoft.com