-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Replace hostname -f with unmae -n #1597
Conversation
We are consistent with upstream. Please work with them to switch that, then we can change this once upstream has changed. |
if err != nil { | ||
return "", fmt.Errorf("Couldn't determine hostname: %v", err) | ||
glog.Fatalf("Couldn't determine hostname: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be handled by other code, not here. We do not glog.Fatal in common code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even upstream is using glog.Fatalf
, we should not use it here?
[1] https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/util/node.go#L31-L34
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are doing startup differently - we only use glog.Fatal in very specific places, and everywhere else is required to return an error.
On Apr 6, 2015, at 11:24 AM, Kenjiro Nakayama [email protected] wrote:
In pkg/cmd/server/start/node_args.go:
if err != nil {
return "", fmt.Errorf("Couldn't determine hostname: %v", err)
Even upstream is using glog.Fatalf , we should not use it here?glog.Fatalf("Couldn't determine hostname: %v", err)
[1] https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/util/node.go#L31-L34—
Reply to this email directly or view it on GitHub.
I think I couldn't tell you correctly. My concern is that it is not simple to get the Hostname by On the other hand, |
That may be the case, but we're simply trying to fit with kube. Can you open an upstream issues on Kubernetes about this with this info? I just want to make sure we're consistent. ----- Original Message -----
|
Ah... I understand. Your upstream means Kubernetes. I thought hostname command's upstream. |
Fixed in upstream - kubernetes/kubernetes#7072. Can you update this pull against the code (we might have added hostname -f in a few places) and then I'll review and merge? Thanks for following through on this. |
Regarding to OpenShift, |
LGTM [merge] |
Evaluated for origin up to cb9f1d8 |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/1820/) (Image: devenv-fedora_1449) |
…service-catalog/' changes from b69b4a6c80..b758460ba7 b758460ba7 origin build: modify hard coded path 871582f73a origin build: add origin tooling 9fa4e70 chart changes for v0.1.8 (openshift#1741) cada49c handle instance deletion that occurs during async provisioning or async update (openshift#1587) (openshift#1708) 3032f01 phony output binaries (openshift#1729) 0c98a72 remove last vestiges of glide (openshift#1696) 8435935 Prune vendor (openshift#1739) 0f657ec allow setting go version, clean up alignment 08af73f Disable test-dep target temporarily 41984a5 Check for existing bindings only for instances with DeprovisionStatus == ServiceInstanceDeprovisionStatusRequired. (openshift#1640) 706e555 chart changes for v0.1.7 (openshift#1721) 23644db we inconsistently rm thing with and without docker (openshift#1713) a38092d Chart changes for Release v0.1.6 (openshift#1718) 2fd4ecf Add PodPreset into settings api group (openshift#1694) bac68f4 update docs of developer's guide (openshift#1716) 3200b16 add integration test for proper async binding retry (openshift#1688) 6d809c3 Add custom columns to OpenAPI schema (openshift#1597) fcdefa6 Workaround spf13/viper stringarray bug (openshift#1700) ebbeb8c undo 6bad71d358ad3ad39eb8c003f5807cca1ec1d1e7 (openshift#1714) 1ee9659 Load all client auth plugins in the cli (openshift#1695) b9ad10d must run tests (openshift#1698) c621cdc add stages to Travis REVERT: b69b4a6c80 origin build: modify hard coded path REVERT: 527fac4d02 origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: b758460ba7a45d370da9d5d634e71c16e9eb282a
OpenShift should not depend on
hostname
command.From RHEL 7 hostnamectl is used for controlling the HostName. In addition, the specification of
hostname
is being changed by upstream often.And then, in case
uname -n
(even ifhostname -f
) returns error, it means Fatal. I think it is impossible to return error byuname -n
(orhostname -f
).