New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for dns-search and fixes #30102 #30117

Merged
merged 2 commits into from Jan 31, 2017

Conversation

@msabansal
Contributor

msabansal commented Jan 13, 2017

- What I did
This PR adds support for --dns-search option for windows. It also fixes #30102 Support for dns-search  requires platform support and will only work with the next version of windows.

- How I did it

- How to verify it
Verified the bug fix using the steps Jason mentioned and also validated that dns-suffix is set on the latest builds.
- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@msabansal msabansal changed the title from Added support for dns-suffix and fixes #30102 to Added support for dns-search and fixes #30102 Jan 13, 2017

@msabansal

This comment has been minimized.

Show comment
Hide comment
@msabansal

msabansal Jan 13, 2017

Contributor

This will require latest windows platforms to work. It will not throw any error on older builds but the option will be silently ignored. This is the present behavior. Do we want to change it so that specifying --dns-search throws error on unsupported platforms?

Contributor

msabansal commented Jan 13, 2017

This will require latest windows platforms to work. It will not throw any error on older builds but the option will be silently ignored. This is the present behavior. Do we want to change it so that specifying --dns-search throws error on unsupported platforms?

Show outdated Hide outdated daemon/container_operations.go Outdated
Show outdated Hide outdated vendor.conf Outdated
Show outdated Hide outdated vendor/github.com/Microsoft/hcsshim/interface.go Outdated
Show outdated Hide outdated vendor/github.com/Microsoft/hcsshim/interface.go Outdated
Show outdated Hide outdated daemon/daemon_windows.go Outdated
Show outdated Hide outdated libcontainerd/types_windows.go Outdated
@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jan 13, 2017

Contributor

This will require latest windows platforms to work. It will not throw any error on older builds but the option will be silently ignored. This is the present behavior. Do we want to change it so that specifying --dns-search throws error on unsupported platforms?

I think that would probably be a good idea.

Contributor

jhowardmsft commented Jan 13, 2017

This will require latest windows platforms to work. It will not throw any error on older builds but the option will be silently ignored. This is the present behavior. Do we want to change it so that specifying --dns-search throws error on unsupported platforms?

I think that would probably be a good idea.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jan 13, 2017

Contributor

Does this also require any doc updates?

Contributor

jhowardmsft commented Jan 13, 2017

Does this also require any doc updates?

msabansal and others added some commits Jan 13, 2017

Added support for dns-search and fixes #30102
Signed-off-by: msabansal <sabansal@microsoft.com>
Vendoring latest hcsshim
Signed-off-by: Sandeep Bansal <msabansal@microsoft.com>
@msabansal

This comment has been minimized.

Show comment
Hide comment
@msabansal

msabansal Jan 13, 2017

Contributor

@jhowardmsft Not sure if it requires a doc update. Perhaps we can add somewhere that dns-search is only supported for rs2. Since we are now throwing an error it should be alright.

Contributor

msabansal commented Jan 13, 2017

@jhowardmsft Not sure if it requires a doc update. Perhaps we can add somewhere that dns-search is only supported for rs2. Since we are now throwing an error it should be alright.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jan 13, 2017

Contributor

LGTM. Thanks for doing the updates @msabansal

Contributor

jhowardmsft commented Jan 13, 2017

LGTM. Thanks for doing the updates @msabansal

@johnstep

LGTM

It might be better to separate the fix for #30102, though.

// encountered if it doesn't already exist
if !defaultNetworkExists &&
runconfig.DefaultDaemonNetworkMode() == containertypes.NetworkMode(strings.ToLower(v.Type)) &&
n == nil {

This comment has been minimized.

@johnstep

johnstep Jan 13, 2017

Contributor

Maybe check n == nil first, or at least second.

@johnstep

johnstep Jan 13, 2017

Contributor

Maybe check n == nil first, or at least second.

This comment has been minimized.

@LK4D4

LK4D4 Jan 30, 2017

Contributor

ping @msabansal

@LK4D4

LK4D4 Jan 30, 2017

Contributor

ping @msabansal

@msabansal

This comment has been minimized.

Show comment
Hide comment
@msabansal

msabansal Jan 21, 2017

Contributor

@jhowardmsft @johnstep Can we merge or do I split these?

The test failures don't look related to this change. Can we rerun or should I triage?

Contributor

msabansal commented Jan 21, 2017

@jhowardmsft @johnstep Can we merge or do I split these?

The test failures don't look related to this change. Can we rerun or should I triage?

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 30, 2017

Contributor

@msabansal looks like @johnstep has minor comment for you

Contributor

LK4D4 commented Jan 30, 2017

@msabansal looks like @johnstep has minor comment for you

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Jan 31, 2017

Member

@LK4D4 code looks good and the comment is really a nit. Let's merge this and do a quick follow-up 👼

Member

vdemeester commented Jan 31, 2017

@LK4D4 code looks good and the comment is really a nit. Let's merge this and do a quick follow-up 👼

@vdemeester vdemeester merged commit c0a1d2e into moby:master Jan 31, 2017

5 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 29880 has succeeded
Details
janky Jenkins build Docker-PRs 38481 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 2760 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 9228 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Jan 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment