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

Makes NewNginxParser tolerant of log_format directives with newlines #44

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dcarney
Copy link

@dcarney dcarney commented Jun 4, 2019

This PR makes slight modifications to the regex used in the NewNginxParser func, so that it correctly parses nginx log_format directives that contain newlines after the name.

This is best illustrated with an example.

This is perfectly valid nginx config (elided for brevity):

http {
  ...

  log_format main 
    '$remote_addr - $remote_user [$time_local] '
    '"$request" $status '
    '"$http_referer" "$http_user_agent"';

  ... 
}

Previously, the NewNginxParser regex would not properly match the log_format main line in such a case, since it expected to match one or more characters in a capture group before the end of the line. This would work if we "hacked" the line to include trailing whitespace, but that solution is error-prone and literally invisible to the naked eye. 😄

This PR changes that, and makes one more trivial change to that particular regex: it uses the standard %s formatting verb when building the regex, rather than the %v.

The rest of the PR consists of comments and additional test cases.

Let me know if you have any concerns, or if you'd like further modifications to be made. Thanks for looking!

@satyrius
Copy link
Owner

Thanks the the fix. Could you please make Travis green before we merge it?

@bobby-stripe
Copy link

to swoop in here, I just looked - I think the Travis troubles are larger/independent of this PR and due to Go 1.13 modules (only tip is failing, and the last numbered Go version tested is Go 1.5):

$ make examples
go run ./example/common/common.go
unexpected directory layout:
	import path: _/home/travis/gopath/src/github.com/satyrius/gonx
	root: /home/travis/gopath/src
	dir: /home/travis/gopath/src/github.com/satyrius/gonx
	expand root: /home/travis/gopath
	expand dir: /home/travis/gopath/src/github.com/satyrius/gonx
	separator: /
make: *** [examples] Error 1
The command "make examples" exited with 2.

@dcarney
Copy link
Author

dcarney commented Jun 24, 2019

@satyrius What do you think about the comment above from @bobby-stripe ?

It does appear that the CI failure is independent of my particular PR changes. Also, it would likely be helpful to change the CI build to test new Go versions > 1.5. I think that should be it's own PR however, instead of being co-mingled into this PR.

Let me know what you think, so that we can keep progress moving towards getting this PR merged. Thanks!

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.

None yet

3 participants