Skip to content

Windows: Refactor network modes#14133

Merged
calavera merged 1 commit intomoby:masterfrom
microsoft:10662-netmode
Jun 29, 2015
Merged

Windows: Refactor network modes#14133
calavera merged 1 commit intomoby:masterfrom
microsoft:10662-netmode

Conversation

@lowenna
Copy link
Copy Markdown
Member

@lowenna lowenna commented Jun 23, 2015

Signed-off-by: John Howard jhoward@microsoft.com

@swernli @mavenugo @mrjana

This PR is part of the proposal described in docker/docker issue 10662 to port the docker daemon to Windows. This refactors the network modes for Windows support.

@calavera
Copy link
Copy Markdown
Contributor

LGTM

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if hostConfig == nil || hostConfig.Links == nil {
    return nil
}

Because the rest of the code is the import stuff, nice to have one less indentation

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Signed-off-by: John Howard <jhoward@microsoft.com>
@lowenna
Copy link
Copy Markdown
Member Author

lowenna commented Jun 29, 2015

@cpuguy83 Comments addressed :) Waiting for Jenkins

@cpuguy83
Copy link
Copy Markdown
Member

LGTM, though naming is a bit funky.
Thanks @jhowardmsft

@calavera Still good for you?

@lowenna
Copy link
Copy Markdown
Member Author

lowenna commented Jun 29, 2015

Easy to rename, but I'm struggling to come up with something better? Open to suggestions :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of default mode to have multiple parts ?
In the case of Linux, multiple parts separated by ":" is used for container namespace. which is not applicable for windows.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Purely to keep it in sync with Linux (for now). You're right, none of this does anything quite yet - I can remove that once we have real code in there. I just wanted to get us off overloading 'bridge' in the first instance.

@mavenugo
Copy link
Copy Markdown
Contributor

@jhowardmsft reviewed it only from the perspective of making sure the linux side of things stay as is. just a minor comment. otherwise LGTM.
I guess these Modes doesnt make much sense to windows yet.

@calavera
Copy link
Copy Markdown
Contributor

Still LGTM. Merging! thank you 🎆

calavera added a commit that referenced this pull request Jun 29, 2015
Windows: Refactor network modes
@calavera calavera merged commit 18d5d3b into moby:master Jun 29, 2015
@lowenna lowenna deleted the 10662-netmode branch July 10, 2015 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants