DNS default check should not be in server.Config

[smarterclayton]

Prints excess messages anytime the CLI is loaded

@deads2k review

[deads2k]

This validation should happen in a separate method call for GetDNSAddr like we do for other addresses that require detection. https://github.com/openshift/origin/blob/master/pkg/cmd/server/config.go#L123 as an example.

[smarterclayton]

It's not appropriate for it to be called more than once, so I don't want to encourage reuse.

On Mar 10, 2015, at 7:46 AM, David Eads [email protected] wrote:

In pkg/cmd/server/origin_master.go:

@@ -83,10 +85,18 @@ func (cfg Config) BuildOriginMasterConfig() (*origin.MasterConfig, error) {
return nil, err
}

• dnsAddr := cfg.DNSBindAddr
  This validation should happen in a separate method call for GetDNSAddr like we do for other addresses that require detection. https://github.com/openshift/origin/blob/master/pkg/cmd/server/config.go#L123 as an example.

—
Reply to this email directly or view it on GitHub.

[liggitt]

this feels similar to the warnings you get when :80 and :443 can't be bound to, but those happen at start time. I would move this earlier (to a validate func) or later (to the start func). Doing it in buildconfig feels weird

[smarterclayton]

Fixed

----- Original Message -----

@@ -83,10 +85,18 @@ func (cfg Config) BuildOriginMasterConfig()
(*origin.MasterConfig, error) {
return nil, err
}

• dnsAddr := cfg.DNSBindAddr

this feels similar to the warnings you get when :80 and :443 can't be bound
to, but those happen at start time. I would move this earlier (to a validate
func) or later (to the start func). Doing it in buildconfig feels weird

---

Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1263/files#r26119239

[deads2k]

One comment, but this is getting nuked one more time to properly delegate commands in the order we want with reusable code and eliminating at least one translation layer in #1247, so merging as-is would be ok.

[deads2k]

@csrwng

[deads2k]

lgtm [merge]

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1171/) (Image: devenv-fedora_1036)

[smarterclayton]

[test]

[openshift-bot]

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_openshift3/1298/)

[smarterclayton]

[merge]

[openshift-bot]

Evaluated for origin up to 3befb47