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

Fix build without Arrow #1673

Merged
merged 10 commits into from May 21, 2021
Merged

Fix build without Arrow #1673

merged 10 commits into from May 21, 2021

Conversation

dominiklohmann
Copy link
Member

馃摂 Description

This fixes the build without Arrow and tests it in CI.

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

n/t

@dominiklohmann dominiklohmann added the bug Incorrect behavior label May 21, 2021
@dominiklohmann dominiklohmann requested a review from lava May 21, 2021 06:47
@dominiklohmann dominiklohmann changed the title Topic/without arrow Fix build without Arrow May 21, 2021
@lava lava force-pushed the topic/without-arrow branch 2 times, most recently from 3dab56e to 6c962c9 Compare May 21, 2021 11:10
@lava
Add special-case checking macros for caf::expected
d386b05 
Add macros that print the error message if a `caf::expected`
unexpectedly returns an error.
lava added 2 commits May 21, 2021 14:26
@lava
Only run 'hash' step in integration test when arrow is enabled
8572426
@lava
Simplify ifdef-sections for transforms
bac15ff 
The idea is that writing an arrow-only transform should be seamlessly
supported for the plugin author, even if it introduces some additional
logic on the processing side in VAST.
dominiklohmann
dominiklohmann commented May 21, 2021
View reviewed changes
Copy link
Member Author

@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.

LGTM except for two things: A minor comment, and please add a separate changelog entry considering this was after the rc1 release.

I obviously cannot approve this myself, so consider this approval, approve yourselves and feel free to merge.

vast/integration/vast_integration_suite.yaml Outdated Show resolved Hide resolved
lava
lava approved these changes May 21, 2021
View reviewed changes
Copy link
Member

@lava lava left a comment

Choose a reason for hiding this comment

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

Approving myself after getting informal approval from @dominiklohmann .

@lava lava enabled auto-merge May 21, 2021 13:38
@lava lava merged commit 65ecfc0 into master May 21, 2021
@lava lava deleted the topic/without-arrow branch May 21, 2021 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lava lava lava approved these changes

Assignees
No one assigned
Labels
bug Incorrect behavior
Projects
None yet
Milestone
No milestone
2 participants
@dominiklohmann @lava