Skip to content
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 support for custom REST handlers without UI in web.conf and restmap.conf #1532

Open
wants to merge 5 commits into
base: feat/handlers-custom-logic
Choose a base branch
from

Conversation

kkedziak-splunk
Copy link
Contributor

@kkedziak-splunk kkedziak-splunk commented Jan 20, 2025

Issue number: ADDON-75952

PR Type

What kind of change does this PR introduce?

  • Feature
  • Bug Fix
  • Refactoring (no functional or API changes)
  • Documentation Update
  • Maintenance (dependency updates, CI, etc.)

Summary

Changes

Second PR. It allows to specify REST handlers not generated by UCC in globalConfig. Based on that, UCC will extend openapi.json, web.conf and restmap.conf.

User experience

Nothing changes in existing addons. This feature introduces new options in global config.

Checklist

If an item doesn't apply to your changes, leave it unchecked.

@kkedziak-splunk kkedziak-splunk requested review from a team as code owners January 20, 2025 13:21
@kkedziak-splunk kkedziak-splunk marked this pull request as draft January 20, 2025 13:21
@kkedziak-splunk kkedziak-splunk changed the title Feat/handlers custom logic 2 feat: add support for custom REST handlers without UI in web.conf and restmap.conf Jan 20, 2025
@kkedziak-splunk
Copy link
Contributor Author

Suggested pull request title: feat: allow definition of custom REST handlers in global config

I appreciate the thorough implementation of support for custom REST handlers in the global configuration. The code is well-structured and includes comprehensive test coverage. The implementation follows good practices by separating concerns into appropriate classes and methods.

However, there are a few areas that could be improved:

  1. The copyright year in splunk_add_on_ucc_framework/commands/rest_builder/user_defined_rest_handlers.py is set to 2025, which seems incorrect. It should probably be 2023 or 2024.

  2. In user_defined_rest_handlers.py, there are some commented-out type hints in docstrings that could be cleaned up or properly documented.

  3. Consider adding more detailed docstrings to the main classes (RestHandlerConfig and UserDefinedRestHandlers) explaining their purpose and usage.

  4. In test_user_defined_rest_handlers.py, consider grouping related test cases using pytest.mark.parametrize to reduce code duplication, especially in the duplicates testing section.

  5. The schema.json changes could benefit from more detailed descriptions in the documentation strings to help users understand the new configuration options.

Overall, the code appears to be ready for merging after addressing these minor issues. The implementation provides a useful feature for defining custom REST handlers without requiring UI components, and it's well-tested with both unit tests and integration tests.

This comment was added by our PR Review Assistant Bot. Please kindly acknowledge that
while we're doing our best to keep these comments up to very high standards, they may occasionally be
incorrect. Suggestions offered by the Bot are only intended as points for consideration and no statements
by this bot alone can be considered grounds for merging of any pull request. Remember to seek a review
from a human co-worker.

To reply to the review and engage Review Bot in further conversation, start your comment with the words Review bot:

@kkedziak-splunk kkedziak-splunk marked this pull request as ready for review January 22, 2025 09:45
# Conflicts:
#	tests/testdata/test_addons/package_global_config_everything/globalConfig.json
@kkedziak-splunk kkedziak-splunk changed the base branch from develop to feat/handlers-custom-logic February 6, 2025 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant