-
Notifications
You must be signed in to change notification settings - Fork 103
[sql-34]: actions: some basic clean-up of the firewall actions code #1065
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
Conversation
In preparation for adding a SQL implementation of the the Actions DB, we move all kvdb CRUD code to a dedicated file.
To better represent the interface and to free up the use of the ActionsDB name as this will be used to represent the full Actions DB in an upcoming commit.
The SessionID is already present in the Action itself and so this does not need to be passed in as its own parameter.
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, LGTM 🔥🚀!
@@ -48,7 +48,7 @@ type ConfigImpl struct { | |||
|
|||
// ActionsDB can be used by rules to list any past actions that were | |||
// made for the specific session or feature. | |||
ActionsDB firewalldb.ActionsDB | |||
ActionsDB firewalldb.ActionsListDB |
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.
nit: Could potentially rename the ActionsDB
field in the config as well as the GetActionsDB
in the interface as it's publicly exposed? I don't know what your upcoming commit that will use the ActionsDB
name to represent the full Actions DB looks like though.
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.
Naming it ActionsLister
could also be an idiomatic way to name it (but I'm fine with the name).
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.
cool - gonna leave rename till the end of the series if that's ok! just realised it causes quite a lot of merge conflicts if i change this now
@@ -48,7 +48,7 @@ type ConfigImpl struct { | |||
|
|||
// ActionsDB can be used by rules to list any past actions that were | |||
// made for the specific session or feature. | |||
ActionsDB firewalldb.ActionsDB | |||
ActionsDB firewalldb.ActionsListDB |
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.
Naming it ActionsLister
could also be an idiomatic way to name it (but I'm fine with the name).
This PR adds some initial clean up of the actions code in preparation for adding a SQL implementation.
Part of #917