-
Notifications
You must be signed in to change notification settings - Fork 20
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
Make logging requests configurable #51
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR, I've left a few comments.
e7496dc
to
5aa567e
Compare
main.go
Outdated
fi, err := os.Stat(logAddress) | ||
if err != nil { | ||
// maybe it's a syslog host[:port] | ||
if len(strings.Split(logAddress, ":")) == 1 { |
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.
Use net.SplitHostPort()
here instead to properly handle IPv6
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.
Thanks! But it turned out to be complicated to check if the error out of net.SplitHostPort()
is "missing port" so I did something else, I think that it's even more elegant. :) Can you please check this code now?
I also added one more case that HAproxy supports - file descriptor/stdout/stderr.
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.
Is the current validation code ok for you, @ShimmerGlass ?
5c55e55
to
8076e1d
Compare
not requiring the app logging level to be set to TRACE and with configurable log address (stdout/stderr/syslog etc.)
I think that in the long term work on making the |
} | ||
} else { | ||
if fi.Mode()&os.ModeSocket == 0 { | ||
return errors.New(fmt.Sprintf("%s is a file but not a socket", logAddress)) |
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.
You can use fmt.Errorf()
to directly create an error with formatting
// allowed values taken from https://cbonte.github.io/haproxy-dconv/2.0/configuration.html#4.2-log | ||
fi, err := os.Stat(logAddress) | ||
if err != nil { | ||
match, err := regexp.Match(`(fd@<[0-9]+>|stdout|stderr)`, []byte(logAddress)) |
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.
Please use regex.MatchString
to remove the type casting
@gdubicki if you'd still like to incorporate the suggestions by Thorleon we could proceed with merging this. |
...not requiring the app logging level to be set to TRACE and with configurable log address - syslog socket.
I think that this fixes #45.
As this is my first contribution and one of the first golang programming attempts please do not hesitate to write me everything that I should fix before this can be merged - I will do by best to apply all the comments. :)