-
Notifications
You must be signed in to change notification settings - Fork 103
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 kolide_snap_upgradeable
table and new data_table
exec parser
#1636
Conversation
StringDelimitedLineFunc
method and kolide_snap_upgradeable
table
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 -- just one nitpick about casing
StringDelimitedLineFunc
method and kolide_snap_upgradeable
tablekolide_snap_upgradeable
table and StringDelimitedLineFunc
method
722856e
to
0782fde
Compare
kolide_snap_upgradeable
table and StringDelimitedLineFunc
methodkolide_snap_upgradeable
table and new data_table
exec parser
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 think this looks pretty good. I like the idiom of New
with options to create a parser. I think it gives us lots of rope.
I'm not totally sold on all the options, and I suspect we'll eventually want functional arguments there. But you've got good test cases, so it's easy to refactor later.
Oh, please rename Parser
to NewParser
or New
before merging.
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 think this is okay. A couple of nits, but you could ignore them
I've created a new string delimited method for processing csv-esque/data table output.
Initially I was aiming to utilize this new method for both
flatpak
andsnap
, but due to many issues that arose withflatpak
(they are commented in that PR) I've only stuck withsnap
for now.