Uh oh!
There was an error while loading. Please reload this page.
Update to Windows network options - #801
Conversation
crosbymichael
commented
May 10, 2017
Doesn't the |
darstahl
commented
May 10, 2017
Yep. Updating that now. |
darstahl
commented
May 10, 2017
Updated |
| } | ||
| "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.
| 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.
lowenna
commented
May 15, 2017
@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? |
wking
commented
May 15, 2017
via email
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) |
lowenna
commented
May 23, 2017
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 |
darstahl
commented
May 23, 2017
Rebased |
lowenna
commented
May 23, 2017
Sorry, needs one more rebase..... |
Signed-off-by: Darren Stahl <darst@microsoft.com>
darstahl
commented
May 23, 2017
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