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

strip \r and \n when reading the file /etc/host_hostname #335

Closed
wants to merge 2 commits into from

Conversation

nvanheuverzwijn
Copy link
Contributor

@nvanheuverzwijn nvanheuverzwijn commented Oct 12, 2017

related to #329

Copy link
Member

@michaelshobbs michaelshobbs left a comment

Choose a reason for hiding this comment

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

would be great if there was a test to cover this. @nvanheuverzwijn do you have a few minutes to add that?

@nvanheuverzwijn
Copy link
Contributor Author

@michaelshobbs Will do. However, testing this might be difficult because of the hardcoded filename. I will make something up.

@nvanheuverzwijn
Copy link
Contributor Author

@michaelshobbs Done. Waiting for your feedback.

t.Fatal(err)
}
done := make(chan string)
addr, sock, srvWG := startServer("tcp", "", done)
Copy link
Member

Choose a reason for hiding this comment

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

looks like that broke the reconnect test. No need to spin up the server. I'd just move the hostname logic to it's own function and test that specifically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, I'm sorry, I really don't have time to do this. Maybe someone with more experience in go should do this.

Copy link
Member

Choose a reason for hiding this comment

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

No worries. I'll take it from here. Thanks for the initial contribution!

@nvanheuverzwijn
Copy link
Contributor Author

Duplicate #338

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.

2 participants