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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make it possible to dump concepts #1196

Merged
merged 12 commits into from Nov 25, 2020
Merged

Make it possible to dump concepts #1196

merged 12 commits into from Nov 25, 2020

Conversation

dominiklohmann
Copy link
Member

@dominiklohmann dominiklohmann commented Nov 24, 2020 •
edited

馃摂 Description

This implements a command that dumps the known concepts without requiring the work to do a full status report.

馃摑 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

Commit-by-commit.

@dominiklohmann dominiklohmann added the feature New functionality label Nov 24, 2020
@dominiklohmann dominiklohmann marked this pull request as ready for review November 24, 2020 17:47
@dominiklohmann dominiklohmann requested a review from a team November 24, 2020 17:55
mavam
mavam approved these changes Nov 24, 2020
View reviewed changes
Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

Nice work so far! Looking forward to reading the documentation of the command.

libvast/src/system/node.cpp Outdated Show resolved Hide resolved
libvast/src/system/node.cpp Outdated Show resolved Hide resolved
mavam
mavam requested changes Nov 25, 2020
View reviewed changes
Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

Mostly nits.

doc/cli/vast-count.md Outdated Show resolved Hide resolved
doc/cli/vast-count.md Outdated Show resolved Hide resolved
doc/cli/vast-count.md Outdated Show resolved Hide resolved
doc/cli/vast-dump-concepts.md Outdated Show resolved Hide resolved
doc/cli/vast-dump.md Outdated Show resolved Hide resolved
doc/cli/vast-dump.md Outdated Show resolved Hide resolved
doc/cli/vast-dump.md Show resolved Hide resolved
libvast/src/system/application.cpp Outdated Show resolved Hide resolved
mavam
mavam approved these changes Nov 25, 2020
View reviewed changes
Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

Looks good. Green light modulo two details:

  1. The changelog entry
  2. The request + actor_cast issue

CHANGELOG.md Outdated Show resolved Hide resolved
@dominiklohmann
Copy link
Member Author

2. The request + actor_cast issue

Let's not have this block the PR, it really is a non-issue; instead, we'll move forward with this as is once CI ran through.

@mavam
Copy link
Member

mavam commented Nov 25, 2020

Let's not have this block the PR, it really is a non-issue; instead, we'll move forward with this as is once CI ran through.

Fine by me iff you create a follow-up story for it.

@dominiklohmann
Copy link
Member Author

Fine by me iff you create a follow-up story for it.

Actually figured this out myself: It doesn't work with stateful pointers only. Pushed a fix.

@dominiklohmann
Remove unnecessary actor cast
fa3fd4b 
Request does not work on stateful actor pointers.
@dominiklohmann dominiklohmann merged commit 5938b2a into master Nov 25, 2020
@dominiklohmann dominiklohmann deleted the topic/dump-concepts-prototype branch November 25, 2020 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mavam mavam mavam approved these changes

Assignees
No one assigned
Labels
feature New functionality
Projects
None yet
Milestone
No milestone
2 participants
@dominiklohmann @mavam