-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: add custom search command support #1672
base: feat/split-custom-search-command
Are you sure you want to change the base?
feat: add custom search command support #1672
Conversation
from splunklib.searchcommands import \ | ||
dispatch, ReportingCommand, Configuration, Option, validators | ||
|
||
{% if import_map %} |
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 am not sure about this need of checking whether we can import "map". One of concerns is that importing a module may create unexpected effects and in my opinion we could avoid that.
I tested locally in ipython some possibilities:
So basically we could change this code a bit:
...
from {{ imported_file_name }} import reduce
try:
from {{ imported_file_name }} import map as module_map
except ImportError:
module_map = None
@Configuration()
class {{class_name}}Command(ReportingCommand):
...
if module_map is not None:
@Configuration()
def map(self, events):
return module_map(self, events)
...
Alternatively:
...
import {{ imported_file_name }} as module
@Configuration()
class {{class_name}}Command(ReportingCommand):
...
if hasattr(module, "map"):
...
In both solutions the condition will be executed only once, when the class is constructed.
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.
There is something about the files that are generated but what about adding just a few words what the command usage looks like from the user perspective?
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.
Okay, will update the last given example and add a screenshot of the search query.
- For `Generating` command, the Python file must include a `generate` function. | ||
- For `Streaming` command, the Python file must include a `stream` function. | ||
- For `Eventing` command, the Python file must include a `transform` function. | ||
- For `Reporting` command, the Python file must include a `reduce` function, and optionally a `map` function if a streaming pre-operation is required. |
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 see that "Reporting" has been changed to "Transforming":
Note: In earlier versions of Splunk Enterprise, transforming commands were referred to as "reporting commands."
(source)
Shall we use the newer name? Or at least mention it?
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.
Also, the "Eventing" can also be called "Dataset processing" (as seen here)
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.
Will change the logic and update the documentation as well.
# TODO: Update the list as and when Splunk introduces new commands. | ||
# Links to use: https://docs.splunk.com/Documentation/SplunkCloud/latest/SearchReference | ||
# https://docs.splunk.com/Documentation/Splunk/latest/SearchReference | ||
SPLUNK_COMMANDS = [ |
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 am not sure about maintaining this list.
Do we really want to prevent users from overwriting commands? Is it really forbidden?
If we want to maintain this list, we should automate it as much as possible. For example, when should we check it?
If we want to keep this list, then some test would be useful, e.g.:
import re
import urllib.request
from splunk_add_on_ucc_framework.const import SPLUNK_COMMANDS
def test_command_list_up_to_date():
with urllib.request.urlopen("https://docs.splunk.com/Documentation/Splunk/latest/SearchReference") as resp:
content = resp.read().decode()
search_commands_ul = re.search(r"Search\s+Commands.+?<ul.+?>(.+?)</ul>", content, re.S).group(1)
search_commands = re.findall(r"<li[^>]*>.*?<a[^>]*>\s*([^\s<]+)\s+?</a>", search_commands_ul, re.S)
assert set(search_commands) == set(SPLUNK_COMMANDS)
This would alert us that the list is not up to date and unfortunately we would have failures when the website changes.
Btw. I ran this code and it seems there are already 4 entries missing 😄
E AssertionError: assert {'abstract', ...efields', ...} == {'abstract', ...efields', ...}
E
E Extra items in the left set:
E 'snoweventstream'
E 'snowevent'
E 'snowincidentstream'
E 'snowincident'
E Use -v to get more diff
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.
It's a great idea to have this test case. However, this 4 mentioned commands are only for ServiceNow
add-on. That's why I had omitted it from the list.
|
||
def _set_attributes(self, **kwargs: Any) -> None: | ||
self.conf_file = "commands.conf" | ||
if self._global_config and self._global_config.has_custom_search_commands(): |
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.
What about changing this line similarly to one of previous PR's?
I.e. instead of:
if self._global_config and self._global_config.has_custom_search_commands():
do:
if self._global_config.has_custom_search_commands():
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, I’ll remove the condition.
This change was part of PR #1671, and since it’s now merged, I’ll update it here as well.
@@ -523,6 +523,7 @@ def generate( | |||
app_manifest=app_manifest, | |||
addon_version=addon_version, | |||
has_ui=global_config.meta.get("isVisible", True), | |||
custom_search_commands=global_config.custom_search_commands, |
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.
Shouldn't a generator take that directly from the global config?
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, will remove it.
In previous implementation it was useful. But as PR #1671 is now merged, now it doesn't add a value.
if arg["validate"].get("minimum") and arg["validate"].get("maximum"): | ||
arg_str += f", validate=validators.{arg['validate']['type']}" | ||
arg_str += f"(minimum={arg['validate'].get('minimum')}, " | ||
arg_str += f"maximum={arg['validate'].get('maximum')})" | ||
elif arg["validate"].get("minimum") and not arg["validate"].get( | ||
"maximum" | ||
): | ||
arg_str += f", validate=validators.{arg['validate']['type']}" | ||
arg_str += f"(minimum={arg['validate'].get('minimum')})" | ||
elif not arg["validate"].get("minimum") and arg["validate"].get( | ||
"maximum" | ||
): | ||
arg_str += f", validate=validators.{arg['validate']['type']}" | ||
arg_str += f"(maximum={ arg['validate'].get('maximum')})" | ||
else: | ||
arg_str += f", validate=validators.{arg['validate']['type']}()" |
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 looks too complicated and I needed a few seconds to read it.
What do you think about simplifying it?
Example:
validator_kwargs = {}
for param in ("minimum", "maximum"):
value = arg["validate"].get(param)
if value:
validator_kwargs[param] = value
validator_kwargs = ", ".join(f"{name}={value}" for name, value in validator_kwargs.items())
validator_arg = f"validators.{arg['validate']['type']}({validator_kwargs})"
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.
Sure, I will try to simplify the implementation.
Issue number: ADDON-76780
PR Type
What kind of change does this PR introduce?
Summary
Added support for
custom search command
. This PR contains the dev changes for custom search command.Test case PR #1654
Changes
customSearchCommand
tag has been added to the global configuration, allowing users to generate their custom search commands usingucc-gen build
. Users will only need to define the logic for their command and update thecustomSearchCommand
inglobalConfig
.User experience
Users can now generate custom search commands using the
ucc-gen build
command. To do so, they need to define the command logic and update the globalConfig accordingly.Checklist
If an item doesn't apply to your changes, leave it unchecked.
Review
Tests
See the testing doc.
Demo/meeting:
Reviewers are encouraged to request meetings or demos if any part of the change is unclear