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

Make logging requests configurable #51

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gdubicki
Copy link
Contributor

@gdubicki gdubicki commented May 17, 2020

...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. :)

Copy link
Collaborator

@ShimmerGlass ShimmerGlass left a 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.

main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
haproxy/state/upstream.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@gdubicki gdubicki force-pushed the log-requests branch 2 times, most recently from e7496dc to 5aa567e Compare May 20, 2020 20:03
@aiharos
Copy link
Collaborator

aiharos commented May 21, 2020

@gdubicki if you don't mind, can you open a new pull request just for the 2nd commit (5aa567e), we can merge the update to the test prerequisites sooner, while this discussion wraps up?

@gdubicki
Copy link
Contributor Author

@aiharos , sure please see #57 .

main.go Outdated
fi, err := os.Stat(logAddress)
if err != nil {
// maybe it's a syslog host[:port]
if len(strings.Split(logAddress, ":")) == 1 {
Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 ?

main.go Outdated Show resolved Hide resolved
@gdubicki gdubicki force-pushed the log-requests branch 2 times, most recently from 5c55e55 to 8076e1d Compare June 3, 2020 10:39
not requiring the app logging level to be set to TRACE
and with configurable log address (stdout/stderr/syslog etc.)
@gdubicki
Copy link
Contributor Author

gdubicki commented Jun 3, 2020

I think that in the long term work on making the -help output in the README consistent with the code doesn't make sense so I have taken the liberty of removing it. Is that ok with you, @ShimmerGlass ?

gdubicki pushed a commit to egnyte/haproxy-consul-connect that referenced this pull request Jun 17, 2020
}
} else {
if fi.Mode()&os.ModeSocket == 0 {
return errors.New(fmt.Sprintf("%s is a file but not a socket", logAddress))

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))

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

@aiharos
Copy link
Collaborator

aiharos commented Jul 27, 2020

@gdubicki if you'd still like to incorporate the suggestions by Thorleon we could proceed with merging this.

@gdubicki
Copy link
Contributor Author

gdubicki commented Jul 27, 2020

Will do that, @aiharos . Can you please take a look also at my other PR, #59 ? Or perhaps there too @Thorleon can review?

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.

Logs
4 participants