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.

Sign up for GitHub

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

Jump to bottom

Fix file identifier check in lsvast #1123

Merged
merged 1 commit into from
Oct 28, 2020
Merged

Fix file identifier check in lsvast #1123

merged 1 commit into from
Oct 28, 2020

Conversation

lava
Copy link
Member

@lava lava commented Oct 27, 2020 •
edited
Loading

📔 Description

📝 Checklist

  • All user-facing changes have changelog entries.
  • The changes are reflected on docs.tenzir.com/vast, if necessary.
  • The PR description contains instructions for the reviewer, if necessary.

🎯 Review Instructions

mavam
mavam reviewed Oct 27, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@dominiklohmann dominiklohmann added the bug Incorrect behavior label Oct 27, 2020
@dominiklohmann
Copy link
Member

Instead of doing the indentation by hand like this, does it make sense to just use vast::data or caf::settings to store the data in a proper structure, and to print it as JSON using the JSON printer?

You already link against libvast anyways, you might as well use the provided functionality. This is not a requirement for this pull request, but rather a note do this properly later on.

@mavam
Copy link
Member

mavam commented Oct 27, 2020

With vast::data we get YAML and JSON conversion, so that would make sense the most.

@lava
Copy link
Member Author

lava commented Oct 27, 2020

Yes, it makes a lot of sense. I did not have a use case for it yet, but I'll be happy to approve a PR that switches to JSON output.

@dominiklohmann
Copy link
Member

Yes, it makes a lot of sense. I did not have a use case for it yet, but I'll be happy to approve a PR that switches to JSON output.

I'll create a story.

dominiklohmann
dominiklohmann approved these changes Oct 28, 2020
View reviewed changes
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.

Just a slight nit on the changelog. Approving as is, I've tested this locally yesterday. Let's get this in before the release.

CHANGELOG.md Outdated Show resolved Hide resolved
@dominiklohmann dominiklohmann mentioned this pull request Oct 28, 2020
Merged
9 tasks
@lava
Fix file identifier check in lsvast
03f3cdf 
The new `read_flatbuffer_file<>()` function already checks the
file identifier and returns a pointer to the flatbuffer type,
not the raw buffer. Therefore, checking it again can not succeed.

Also, added proper indentation to the output of lsvast.
@lava lava merged commit 0405cd1 into master Oct 28, 2020
@lava lava deleted the topic/lsvast-formatting branch October 28, 2020 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mavam mavam mavam left review comments

@dominiklohmann dominiklohmann dominiklohmann approved these changes

Assignees
No one assigned
Labels
bug Incorrect behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lava @dominiklohmann @mavam