-
Notifications
You must be signed in to change notification settings - Fork 905
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
update: print JSON version output by using json_output config field #2351
Conversation
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
…json_output Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
…zation Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
/milestone 0.34.0 |
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
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.
Minor comments/questions aside, LGTM!
Perhaps @LucaGuerra is interested, because he made the |
…gured Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
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.
/approve
LGTM label has been added. Git tree hash: a2a250a200f96d737b0ec0a5f650b234c7aedab8
|
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP, jasondellaluce The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Can there be an additional documentation update that makes it clear that This was surprising to me initially when pulling in the latest upstream changes because I expected (My confusion was also compounded a user runs |
What type of PR is this?
/kind cleanup
/kind feature
Any specific area of the project related to this PR?
/area engine
/area tests
What this PR does / why we need it:
This PR proposes removing the recently-introduced
version-json
CLI option and makes Falco print a JSON version output depending on thejson_output
configuration field such as follows:The goal here is to avoid duplication of UX elements, since
json_output
is already used other Falco features to control whether the output is in plain text of JSON format.Which issue(s) this PR fixes:
Special notes for your reviewer:
This PR can be the first of a series where we make every Falco output support a JSON format depending on the
json_output
configuration. In the past, that field was only used to control the JSON formatting of the alert outputs, and it recently was also used to output the result of rule files validation in JSON format. Since we now have a third use case, I think in the future we might usejson_output
to make every Falco output formatted as JSON and thus machine-readable.Also, this PR refactors a little bit the
falco_configuration
class (responsible of ~250 LOC to preserve the git blame), which can not be initialized and supports a more explicit default state. Moreover, loading the configuration has been moved as the first action performed by Falco, so that all actions can rely on configuration fields (such asjson_output
). Accordingly, blocking validation checks (such as requiring at least one enabled output or at least one rules file) are moved in their own action, so that early actions such asprint_version
are not blocked and can proceed even with an empty configuration.Does this PR introduce a user-facing change?: