-
Notifications
You must be signed in to change notification settings - Fork 55
Replaces yaml cpp with libyaml in SCA module #840
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
Replaces yaml cpp with libyaml in SCA module #840
Conversation
7d01940
to
e828afd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few notes on const correctness and removal of writes to standard output. We should add tests since it's a lot of code to cover.
try | ||
{ | ||
if (!IsValidYamlFile(filename.string())) | ||
std::cout << "*** PolicyParser(" << filename.c_str() << ")\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove all couts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
{ | ||
std::vector<std::string> values; | ||
for (const auto& item : yamlNode) | ||
auto items = yamlNode.AsSequence(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
if (root.HasKey("variables")) | ||
{ | ||
for (const auto& var : variables) | ||
auto variablesNode = root["variables"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
{ | ||
std::vector<std::string> values; | ||
for (const auto& item : yamlNode) | ||
auto items = yamlNode.AsSequence(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
try | ||
{ | ||
if (!IsValidYamlFile(filename.string())) | ||
std::cout << "*** PolicyParser(" << filename.c_str() << ")\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There shouldn't be any couts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those were in a separate commit ready to be removed, done
return nullptr; | ||
} | ||
|
||
std::cout << "*** Policy parsed successfully\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There a few more couts, can we remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
#include <filesystem> | ||
#include <string> | ||
|
||
class YamlNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this forward declaration needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it isn´t. Removed.
YamlDocument() | ||
: m_loaded(false) {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to the source file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
bool IsValidDocument() const | ||
{ | ||
return m_loaded; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to the source file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
YamlNode() = default; | ||
|
||
/// @brief Gets the type of the YAML node | ||
/// @return Type of the node | ||
Type GetNodeType() const | ||
{ | ||
return m_type; | ||
} | ||
|
||
/// @brief Gets the type of the YAML node as a string | ||
/// @return String representation of the node type | ||
std::string GetNodeTypeAsString() const | ||
{ | ||
switch (m_type) | ||
{ | ||
case Type::Scalar: return "Scalar"; | ||
case Type::Sequence: return "Sequence"; | ||
case Type::Mapping: return "Mapping"; | ||
case Type::Undefined: // fallthrough | ||
default: return "Undefined"; | ||
} | ||
} | ||
|
||
/// @brief Checks if the YAML node is a scalar | ||
/// @return True if the node is a scalar, false otherwise | ||
bool IsScalar() const | ||
{ | ||
return m_type == Type::Scalar; | ||
} | ||
|
||
/// @brief Checks if the YAML node is a sequence | ||
/// @return True if the node is a sequence, false otherwise | ||
bool IsSequence() const | ||
{ | ||
return m_type == Type::Sequence; | ||
} | ||
|
||
/// @brief Checks if the YAML node is a map | ||
/// @return True if the node is a map, false otherwise | ||
bool IsMap() const | ||
{ | ||
return m_type == Type::Mapping; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to src file please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
03e6414
to
a703eff
Compare
25c0516
to
86a66d4
Compare
86a66d4
to
bf23261
Compare
b59153b
to
6838ef0
Compare
7e8df06
into
enhancement/30270-migrate-sca-6x-code-to-5x-and-compile
Description
To port SCA from 6.x to 5.x we need to replace the YAML parsing library used so we avoid adding new external dependencies to 5.x. This replacement needs only to be done for the 5.x migration.
Results and Evidence
YAML Document is loaded and policy parsed successfully:
Tests pass:
YamlDocument and YamlNode tests:
PolicyParser tests are mostly the same as existed in 6.x except for a couple that became irrelevant.
PolicyParserTest:
Full set of agent tests: