-
Notifications
You must be signed in to change notification settings - Fork 34
feat: provide support for custom search command #1534
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
|
Let me review this extensive pull request that adds support for custom search commands. Suggested pull request title: feat: add custom search command support The pull request demonstrates high quality and introduces significant enhancements to the add-on UCC framework. Here's what I appreciate about it:
Here are a few specific suggestions for improvement:
The changes appear suitable for merging once any testing feedback has been addressed. The commits are cleanly organized and the changes ready for review. The best feature of this PR is its comprehensive approach - not just implementing the core functionality but also adding proper tests, documentation and sample code. Great job on the PR organization and quality of implementation. Feel free to merge once CI passes and any testing feedback has been addressed. This comment was added by our PR Review Assistant Bot. Please kindly acknowledge that To reply to the review and engage Review Bot in further conversation, start your comment with the words |
|
Just tested the PR review bot |
| , require=False | ||
| {%- endif -%} | ||
|
|
||
| {%- if arg.validate -%} |
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 is a very complicated IF. Maybe it should be done outside of the template?
It could be passed as a string.
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 is a nested if-else ladder, but yes the conditions are seem long due to the Jinja2 template.
Doing it outside in the Python scripts would work, but I don't think it would be a patch, as the templates should be providing the skeleton and the values then fill in the muscles and organs.
| if (command["requireSeachAssistant"] is False) and ( | ||
| command.get("description") | ||
| or command.get("usage") | ||
| or command.get("syntax") | ||
| ): |
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.
Can you please check is it possible to have this logic in the schema.json? Or would it be too complicated for the JSON schema to handle?
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, it is possible to implement the above logic in JSON schema the reason I think we shouldn't go with schema.json approach is because,
JSON Schemavalidators typically reject invalid data, rather than logging a warning, and in this case I don't think we should stop the build process.JSON Schemalibrary does not support custom error messages directly. It is better to validate and raise warning through above logic.
| " is not been defined in globalConfig. Defined them as requireSeachAssistant is set to True. " | ||
| ) | ||
| sys.exit(1) | ||
| if command["version"] == 1: |
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.
Should we not support version 1? We mention in this PR that it is deprecated.
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.
Though version 1 is deprecated, we should support it as add-ons like ServiceNow and BMC Remedy uses version 1 for their custom search commands, hence I kept 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.
Let's remove it from UCC, I'll create a Jira for the team to move to the version 2.
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.
Btw, I checked both those add-ons and I can't find version in commands.conf. Is there another way to figure out which version do they use?
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.
The determination of version is can be seen based on this description.
Okay, I'll remove the support of version 1 from this PR.
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.
Removed the support of version 1.
| for command in kwargs["custom_search_commands"]: | ||
| self.command_names.append(command["commandName"]) | ||
|
|
||
| def generate_conf(self) -> Union[Dict[str, str], None]: |
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 happens if there is a commands.conf file already in the package folder?
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.
If the commands.conf is present in the package folder, it would overwrite the one we generated, as it is done for the other conf files (except app.conf). Do we want to merge the generated and source content of the conf files?
| src_pkg_bin = os.path.realpath(os.path.join(self._input_dir, "bin")) | ||
| sys.path.insert(0, src_pkg_bin) | ||
| if hasattr(import_module(command["fileName"]), "map"): | ||
| import_map = True | ||
| sys.path.pop(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.
Can you please explain what are we trying to achieve 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.
For reporting type of custom search command, map method is optional, hence we check whether the developer has implemented this map method or not.
|
|
||
| {% if import_map %} | ||
| @Configuration() | ||
| def map(self, events): |
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.
Can't we always add this method, without this check with importing the command? Would it have any drawbacks?
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.
map is an optional method which is used for streaming preop, if there is no need of pre-operation then we can remove this method.
(reference)
kkedziak-splunk
left a comment
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 copied the minimal definition from the docs to splunk-example-ta, created an empty mycommandlogic.py file and built it. I got the following exception:
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "/Users/kkedziak/PycharmProjects/addonfactory-ucc-generator/splunk_add_on_ucc_framework/main.py", line 244, in main
build.generate(
File "/Users/kkedziak/PycharmProjects/addonfactory-ucc-generator/splunk_add_on_ucc_framework/commands/build.py", line 519, in generate
if (command["requireSeachAssistant"] is False) and (
KeyError: 'requireSeachAssistant'
| ) | ||
| sys.exit(1) | ||
|
|
||
| if (command["requireSeachAssistant"] is False) and ( |
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.
| if (command["requireSeachAssistant"] is False) and ( | |
| if (command["requiredSearchAssistant"] is False) and ( |
and typos in other occurences of this phrase (compared with docs)
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.
Thank you for pointing it out, I have made the respective changes in all files.
|
Let's wait with merging this one, I'd like to discuss @kkedziak-splunk's document first and find consensus in the team. |
splunk_add_on_ucc_framework/generators/conf_files/create_searchbnf_conf.py
Outdated
Show resolved
Hide resolved
| files_to_delete.append(sep.join([output_path, ta_name, "bin", "template_rest_handler_script.py"])) | ||
| files_to_delete.append(sep.join([output_path, ta_name, "bin", "file_does_not_exist.py"])) | ||
| files_to_delete.append(sep.join([output_path, ta_name, "default", "nav", "views", "file_copied_from_source_code.xml"])) | ||
| files_to_delete.append(sep.join([output_path, ta_name, "bin", "__pycache__", "sum_without_map.cpython-37.pyc"])) |
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 don't think we should keep this (or TA devs). Maybe we should clean this up?
EDIT: my mistake. Ignore this comment.
Code Coverage
|
| Type | PR | Develop | Change | Status |
|---|---|---|---|---|
| Line Coverage | 86.84% | 86.90% | 0.05% | 🔴 Decreased |
| Branch Coverage | 79.83% | 80.12% | 0.29% | 🔴 Decreased |
| {%- if (arg.validate.type == "Integer" or arg.validate.type == "Float") -%} | ||
| {%- if (arg.validate.minimum and arg.validate.maximum) -%} | ||
| , validate=validators.{{ arg.validate.type }}(minimum={{ arg.validate.minimum }}, maximum={{ arg.validate.maximum }}) | ||
| {%- elif (arg.validate.minimum and not arg.validate.maximum) -%} | ||
| , validate=validators.{{ arg.validate.type }}(minimum={{ arg.validate.minimum }}) | ||
| {%- elif (not arg.validate.minimum and arg.validate.maximum) -%} | ||
| , validate=validators.{{ arg.validate.type }}(maximum={{ arg.validate.maximum }}) | ||
| {%- else -%} | ||
| , validate=validators.{{ arg.validate.type }}() | ||
| {%- endif -%} | ||
| {%- else -%} | ||
| , validate=validators.{{ arg.validate.type }}() | ||
| {%- endif -%} | ||
| {%- endif -%} |
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.
Are we going to move this logic out of the template? I think we agreed to do that in a meeting two weeks ago, but maybe something has changed
|
Closing the PR as it has been broken down into new PRs. |
Issue number:
ADDON-76780
PR Type
What kind of change does this PR introduce?
Summary
Added support for
custom search commandChanges
customSearchCommandtag has been added to the global configuration, allowing users to generate their custom search commands usingucc-gen build. Users only need to define the logic for their command and update thecustomSearchCommandinglobalConfig.User experience
Users can now generate custom search commands using the
ucc-gen buildcommand. 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.