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

Add YAML support #1045

Merged
merged 16 commits into from
Sep 10, 2020
Merged

Add YAML support #1045

merged 16 commits into from
Sep 10, 2020

Conversation

mavam
Copy link
Member

@mavam mavam commented Sep 4, 2020

To be rebased onto master after #1017 is merged. Ignore everything up to and including 6b4e564.

@mavam mavam added blocked Blocked by an (external) issue feature New functionality labels Sep 4, 2020
@mavam mavam force-pushed the story/ch18525 branch 6 times, most recently from 19fe9fe to e7c358c Compare September 6, 2020 04:55
libvast/src/data.cpp Show resolved Hide resolved
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.

Second pass.

libvast/vast/concept/parseable/vast/yaml.hpp Outdated Show resolved Hide resolved
libvast/src/data.cpp Show resolved Hide resolved
libvast/src/data.cpp Outdated Show resolved Hide resolved
libvast/src/data.cpp Outdated Show resolved Hide resolved
@mavam mavam marked this pull request as ready for review September 8, 2020 19:41
@mavam mavam removed the blocked Blocked by an (external) issue label Sep 9, 2020
libvast/src/data.cpp Show resolved Hide resolved
libvast/src/data.cpp Show resolved Hide resolved
return xs;
}
}
die("unhandled YAML node type");
Copy link
Member

Choose a reason for hiding this comment

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

That's pretty harsh, why not just throw YAML::Exception and fail to parse the file?

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is that this is more like a developer issue. We enumerated all switch statements above, so we should never reach this code. If so, it's a logic error. In order to make the compiler happy (and not spit out a warning), I plugged in a [[noreturn]] function for which we conveniently have die.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with leaving it as-is here, but in general we should be careful with jumping from "programmer error" to "kill the database", especially when there's a perfectly reasonable recovery path.

Related rant: https://lkml.org/lkml/2016/10/4/1

@mavam
Copy link
Member Author

mavam commented Sep 10, 2020

If yaml-cpp indeed uses exceptions on the print path, the to_yaml() function below needs a try-catch block as well

@lava I could not find a single throw in the emitter code in yaml-cpp, that's why I didn't wrap it into a try-catch block.

@mavam
Copy link
Member Author

mavam commented Sep 10, 2020

I've added a changelog as well. Before the next release, we may consolidate it with a single entry about the new config file format.

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.

Please install libyaml-cpp-dev in the bundled Dockerfile as well.

@mavam
Copy link
Member Author

mavam commented Sep 10, 2020

Please install libyaml-cpp-dev in the bundled Dockerfile as well.

Why didn't CI complain about this missing dependency, @dominiklohmann?

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 cosmetic requests now. Approving already.

Please fix these and wait for CI before merging, especially because of the Docker CI.

CMakeLists.txt Outdated Show resolved Hide resolved
libvast/vast/concept/parseable/vast/yaml.hpp Show resolved Hide resolved
libvast/vast/data.hpp Outdated Show resolved Hide resolved
libvast/vast/data.hpp Outdated Show resolved Hide resolved
@mavam
Copy link
Member Author

mavam commented Sep 10, 2020

@dominiklohmann I pushed a minor fixup and now the Docker build failed with this error:

Warning: apt-key output should not be parsed (stdout is not a terminal)
Executing: /tmp/apt-key-gpghome.rS81EKvBWf/gpg.1.sh --keyserver hkp://pool.sks-keyservers.net:80 --recv-keys 5C808C2B65558117
gpg: keyserver receive failed: Cannot assign requested address

Any objections if I merge nonetheless?

@mavam
Copy link
Member Author

mavam commented Sep 10, 2020

I got your approval in person, @dominiklohmann. 🙂

@mavam mavam merged commit 91d93aa into master Sep 10, 2020
@mavam mavam deleted the story/ch18525 branch September 10, 2020 12:12
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.

3 participants