Skip to content
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

Always use localhost, not host name #2367

Merged
merged 3 commits into from
Apr 21, 2015
Merged

Always use localhost, not host name #2367

merged 3 commits into from
Apr 21, 2015

Conversation

corylanou
Copy link
Contributor

Fixes #2350

@@ -18,6 +18,8 @@
- [#2338](https://github.com/influxdb/influxdb/pull/2338): Fix panic if tag key isn't double quoted when it should have been
- [#2340](https://github.com/influxdb/influxdb/pull/2340): Fix SHOW DIAGNOSTICS panic if any shard was non-local.
- [#2351](https://github.com/influxdb/influxdb/pull/2351): Fix data race by rlocking shard during diagnostics.
- [#2350](https://github.com/influxdb/influxdb/pull/2350): Issue fix for :influxd -hostname localhost.
Copy link
Contributor

Choose a reason for hiding this comment

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

@corylanou -- this is not the usual style we've been following, unless you are also proposing to merge 2350?

Copy link
Contributor

Choose a reason for hiding this comment

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

#2350 is an issue, not a PR. The usual style is not to add issue URL, but just the PR URL, and say something like "fixes 2350". Unless I am missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was told that based on what Jason/Todd talked about last week, we now add each issue that it fixes, along with the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I thought Jason's change was a once-off, and the format was more like:

#1234 description.....fixes 4678, 3789, 23789

where 1234 is a PR, not an issue.

I think the link is wrong though in your change. It works, but it's actually an issue.

@otoolep
Copy link
Contributor

otoolep commented Apr 21, 2015

+1 on green build.

@neonstalwart
Copy link
Contributor

if c.Hostname, _ = os.Hostname(); c.Hostname == "" {
c.Hostname = "localhost"
}

Copy link
Contributor

Choose a reason for hiding this comment

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

wondering why it wouldn't be good to still have

if c.Hostname == "" {
  c.Hostname = "localhost"
}

i'm probably not thinking about something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

c.Hostname is always "" when you create a new config.

@toddboom
Copy link
Contributor

+1

corylanou added a commit that referenced this pull request Apr 21, 2015
Always use localhost, not host name
@corylanou corylanou merged commit 5203e7f into master Apr 21, 2015
@corylanou corylanou deleted the localhost-2350 branch April 21, 2015 20:29
mark-rushakoff pushed a commit that referenced this pull request Jan 9, 2019
Add org admin and member specific permissions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

influxd -hostname localhost
4 participants