-
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
Store configuration for Kolide custom ATC tables #1761
Conversation
dcc2af1
to
86b7f35
Compare
86b7f35
to
5404575
Compare
ee/agent/knapsack/knapsack.go
Outdated
@@ -87,6 +87,10 @@ func (k *knapsack) AgentFlagsStore() types.KVStore { | |||
return k.getKVStore(storage.AgentFlagsStore) | |||
} | |||
|
|||
func (k *knapsack) AtcConfigStore() types.KVStore { |
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.
This will end up in a new bucket, right? Do we want that vs I-don't-know
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.
Yes, new bucket! I think the other option would be making it an agent flag and it'd live alongside the other agent flags, which I'm not opposed to either. A new bucket and subsystem felt like it gave me a little bit more room to iterate.
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'm game to try....
In a world where we have 4 katc tables, what will that look like? I imagine they'll be a DSL where we have a table name, and then some parameters defining it.
So if that was in the agent flags, we'd have katc => { table1 => {...}, table2 => {...} }
and if it was in a bucket we'll see entries for each table? I guess it's kinda all the same.
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.
Yeah, the options are exactly as you describe. They both feel ok to me -- let's try this and it shouldn't be painful to switch if we change our minds later 🙂
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.
looks good to me!
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.
Nice!
This is part 1 of enabling ATC for non-vanilla sqlite files (indexeddb, sqlite where the data has been compressed with snappy, dataflatten/exec tables).
This PR gets some boilerplate out of the way:
I don't love "Kolide custom ATC" -- I'm happy to rename if anyone has a better suggestion.