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

Implement reader for Syslog RFC5424 #770

Merged
merged 6 commits into from
Mar 16, 2020

Conversation

knapperzbusch
Copy link
Contributor

@knapperzbusch knapperzbusch commented Feb 26, 2020

This PR adds a Syslog parser and reader. With this, it is possible to import Syslog messages in the format defined in RFC5424. If a message can't be parsed, the message will be handled as single string.

@dominiklohmann dominiklohmann added the feature New functionality label Feb 26, 2020
Copy link
Member

@dominiklohmann dominiklohmann 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 your contribution! 🚀

Can you do a first pass of rebasing? Ideally, this would be just five commits:

  1. Add Syslog parser (RFC5424)
  2. Add unit tests for Syslog parser
  3. Add reader for Syslog
  4. Hook reader into command line
  5. Add integration tests for real-world Syslog data

Additionally, please do a pass over your code after reading our contributing guide.

Other than that, this is looking quite good already. I'll do a second pass tomorrow that's more code-specific.

Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

I've done a second pass.

CHANGELOG.md Outdated Show resolved Hide resolved
doc/cli/vast-import-syslog.md Outdated Show resolved Hide resolved
libvast/vast/format/syslog.hpp Outdated Show resolved Hide resolved
libvast/vast/concept/parseable/core/repeat.hpp Outdated Show resolved Hide resolved
libvast_test/src/main.cpp Show resolved Hide resolved
libvast_test/src/main.cpp Show resolved Hide resolved
libvast_test/vast/test/data.hpp Outdated Show resolved Hide resolved
libvast/vast/format/syslog.hpp Outdated Show resolved Hide resolved
libvast/vast/format/syslog.hpp Outdated Show resolved Hide resolved
@knapperzbusch knapperzbusch force-pushed the feature/syslog-parser branch 3 times, most recently from cf1130d to 5ad8c99 Compare March 1, 2020 21:07
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

I have done another pass.

libvast/vast/format/syslog.hpp Outdated Show resolved Hide resolved
libvast/vast/format/syslog.hpp Outdated Show resolved Hide resolved
libvast/vast/format/syslog.hpp Outdated Show resolved Hide resolved
libvast/vast/format/syslog.hpp Outdated Show resolved Hide resolved
libvast/vast/format/syslog.hpp Outdated Show resolved Hide resolved
libvast/vast/format/syslog.hpp Outdated Show resolved Hide resolved
libvast/vast/format/syslog.hpp Outdated Show resolved Hide resolved
libvast/vast/format/syslog.hpp Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
doc/cli/vast-import-syslog.md Outdated Show resolved Hide resolved
@knapperzbusch knapperzbusch force-pushed the feature/syslog-parser branch 2 times, most recently from a7f6dcc to f0d8911 Compare March 4, 2020 12:26
@dominiklohmann
Copy link
Member

@knapperzbusch should this still be in draft mode?

@knapperzbusch knapperzbusch marked this pull request as ready for review March 13, 2020 08:56
@knapperzbusch
Copy link
Contributor Author

I will do rebase with master today night or tomorrow morning, to resolve the conflicts.

@dominiklohmann
Copy link
Member

Sounds good. Will do one last review pass then (likely next week), so we can get this merged.

@dominiklohmann
Copy link
Member

I took the freedom to rebase this once more and rewrite some comments—no more code changes were necessary.

Thanks for your contribution, @knapperzbusch! We'll merge once CI has run through successfully.

Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

Great work! 🚀

@dominiklohmann dominiklohmann merged commit b076a26 into tenzir:master Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants