-
Notifications
You must be signed in to change notification settings - Fork 897
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 cmdline opts library/initial falco application object #1886
Conversation
38d7bd3
to
aaf2447
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.
I took a quick look. I like the idea, but I don't know cxxopts
, so I need some time to play with it a bit :)
Meanwhile, I have left some (not really important) suggestions.
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.
I left you some comments. I like the direction this is going to!
bc552a8
to
84a1b27
Compare
0e2f12b
to
d976874
Compare
Edit: No longer true, we'll keep the single option and users will have to use |
e4af623
to
98741f6
Compare
7314640
to
d1fa33f
Compare
We'll use this to better manage the fairly large set of command line options in self-contained objects instead of a scattering of individual stack variables. Signed-off-by: Mark Stemm <[email protected]>
Add a notion of a falco application object. Eventually this will replace the bulk of falco_init and contain methods to: - Parse/validate command line options - Parse/validate falco config - Initialize prerequsites (inspector, falco engine, webserver, etc) - Load plugins - Load/validate rules - Command/subcommand execution (e.g. --list/--list-fields, or nothing specified to run "main" loop) For now, it is only responsible for command line options handling, which is stubbed out. Currently, the only public methods are init() to initialize everything and copts() to access command line options. Command line options are held in a different class falco::app::cmdline_opts. application::copts() returns a reference to that object, which allows access to parsed command line options bound to various public instance variables. Signed-off-by: Mark Stemm <[email protected]>
ceef0b8
to
8a06300
Compare
8a06300
to
77c8bb7
Compare
Thanks for doing the extra debugging and the patch! I went back to a single option, where |
77c8bb7
to
9c0451f
Compare
Fill in an initial falco::app::cmdline_options class using cxxopts library to hold options: - falco::app::cmdline_options contains a cxxopts::Options object to parse options and a cxxopts::ParseResult to hold the result. - The only meaningful public method is parse() which parses argc/argv and returns true/false + error. - The parsed options are all public instance variables of the object and generally use the same names of the corresponding variables in the old falco_init(). These variables are all bound to the corresponding command line option and are updated in parse(). - In a few cases, the command line option does not directly map to a bound variable (e.g. -b to set buffer format, -p/-pk/-pc to set extra formatting options, etc.) In these cases the option values are read after parsing and update the public instance variable. Signed-off-by: Mark Stemm <[email protected]>
For the most part, replacing getopt() with cxxopts + falco application had no effect on falco engine/config interfaces. However, there were a few places where it was wasier to change the interface than add middleware code that transformed from, for example, vectors to lists. This commit has those changes. Signed-off-by: Mark Stemm <[email protected]>
cabfb62
to
3aeba3e
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.
Thanks for addressing all the comments, this looks much more solid to me! Left you some comments/concerns. I'm not super-strong about those, but I wanted to have some feedback from you on those points.
Update falco's main falco_init() to use a falco::app::application and falco::app::cmdline_opts object instead of storing all its command line state in stack variables. The bulk of the removed code is in usage() (not needed as cxxopt's help() is self-documenting.) and getopt_long() which is replaced by app.init(argc, argv). For the most part, this is simply replacing references to local variables (e.g. "all_events") to the bound variable inside the cmdline_opts object (e.g. app.copts().all_events). There are a few cases where more complex logic was used (output formats, initializing k8s/mesos with string pointers), and those changes are still in falco_init(). For the most part, the monolithic parts of falco_init that involve reading config files, creating the inspector, loading rules, etc are still present. Those will be addressed in later changes. Signed-off-by: Mark Stemm <[email protected]>
3aeba3e
to
5a0e7b9
Compare
/milestone 0.32.0 |
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.
LGTM now.
Thank you, Mark!
/approve
LGTM label has been added. Git tree hash: 4557d2fd39e24792b9e5037da6178965c7a1e4f2
|
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: jasondellaluce, leogr, mstemm 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 |
What type of PR is this?
/kind cleanup
Any specific area of the project related to this PR?
/area engine
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: