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

Enhance dump command to show models #1233

Merged
merged 14 commits into from
Dec 16, 2020
Merged

Enhance dump command to show models #1233

merged 14 commits into from
Dec 16, 2020

Conversation

mavam
Copy link
Member

@mavam mavam commented Dec 15, 2020 •
edited
Loading

📔 Description

This PR enhances the dump command to show models in addition to concepts.

📝 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.
  • Address FIXMEs (empty-string description when dumping)

🎯 Review Instructions

@dominiklohmann:

  • All command-related C++ file-by-file

@tobim:

  • Taxonomy map storage change
  • Integration test suite changes

@mavam
Copy link
Member Author

mavam commented Dec 15, 2020 •
edited
Loading

As the guardian of the integration test suite, @tobim may want to have a look at the corresponding commits.

@mavam mavam requested a review from tobim December 15, 2020 21:39
@mavam mavam marked this pull request as ready for review December 15, 2020 21:39
dominiklohmann
dominiklohmann approved these changes Dec 15, 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.

Code-wise this is fine.

Can you change the documentation of vast dump itself so it reflects that it dumps both models and concepts?

Please wait for @tobim to review the integration test suite changes. I did not look at them.

@mavam
Copy link
Member Author

mavam commented Dec 16, 2020

I'm handing this PR over to @tobim, who is going to investigate the non-determinism during concept dumping.

@mavam mavam assigned mavam and unassigned tobim Dec 16, 2020
@mavam
Copy link
Member Author

mavam commented Dec 16, 2020

Taking the PR back after internal sync.

tobim
tobim previously requested changes Dec 16, 2020
View reviewed changes
Copy link
Member

@tobim tobim 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 suggestions to simplify the python code.

integration/integration.py Outdated Show resolved Hide resolved
integration/integration.py Outdated Show resolved Hide resolved
@mavam
Copy link
Member Author

mavam commented Dec 16, 2020

You only take a const std::regex& here, so you should get away with forward-declaring it.

Is there a header for this, like we have for <iosfwd>?

<regex> is one of the most expensive headers to include, and I'm not sure why we need it here.

It makes for a convenient way to control the size of the return value.

The default value is also crazy expensive. std::regex should never be created on the fly.

We're doing I/O here. And it's happening pretty much once at startup. The performance concerns don't appear to be in relation to the functionality.

dominiklohmann and others added 13 commits December 16, 2020 20:37
@dominiklohmann
Implement dump models subcommand
8569417
@mavam @dominiklohmann
Add CLI documentation
54a8c51
@mavam @dominiklohmann
Enhance related changelog entry
6b3d4fc
@mavam @dominiklohmann
Make test spec a pattern and case-insensitive
20d412c 
Too many times running tests requires fiddling with the exact test name
in the right casing. This commit enhances -t such that it no longer
looks at the casing of the test name.

Moreover, the test specification is now interpreted as full-blown regex
and can match just a fraction of the tests. For example, `-t zeek` runs
all Zeek tests
@mavam @dominiklohmann
Add integration test for model dumping
8c35755
@mavam @dominiklohmann
Sort output only for non-deterministic commands
ed8365b
@mavam @dominiklohmann
Add integration test for dumping concepts
2af55b5
@mavam @dominiklohmann
Fix invalid subcommand definition
c25e087 
Co-authored-by: Dominik Lohmann <mail@dominiklohmann.de>
@mavam @dominiklohmann
Add note on how top-level dump works
e700b6f
@mavam @dominiklohmann
Use deterministic taxonomy storage
2dca1a4
@mavam @dominiklohmann
Make schema loading deterministic
43a122a
@mavam @tobim @dominiklohmann
Simplify integration test suite enhancements
ba4787f 
Co-authored-by: tobim <tobim@fastmail.fm>
@dominiklohmann
Remove outdated integration test files
6ed1771
dominiklohmann
dominiklohmann approved these changes Dec 16, 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.

This works fine.

Not printing empty descriptions / nested concepts / fields is a strict follow up and should not block the imminent release.

The <regex> stuff I'll clean up myself after the release, I created a story for that.

@dominiklohmann dominiklohmann merged commit 2f4d153 into master Dec 16, 2020
@dominiklohmann dominiklohmann deleted the topic/dump-models branch December 16, 2020 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominiklohmann dominiklohmann dominiklohmann approved these changes

@tobim tobim tobim left review comments

Assignees

@mavam mavam

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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