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: REST handlers without UI - OpenApi support #1529

Draft
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

kkedziak-splunk
Copy link
Contributor

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

One of two PR's. It allows to specify REST handlers not generated by UCC in globalConfig. Based on that, UCC will extend openapi.json with additional entries. The second feature will contain the second feature + docs. Nothing changes in existing addons. This feature introduces new options in global config.

@kkedziak-splunk kkedziak-splunk requested review from a team as code owners January 15, 2025 16:37
@kkedziak-splunk kkedziak-splunk marked this pull request as draft January 15, 2025 16:38
@kkedziak-splunk
Copy link
Contributor Author

kkedziak-splunk commented Jan 16, 2025

Description temporarily changed for running the review bot.

@kkedziak-splunk
Copy link
Contributor Author

Suggested pull request title: feat: add support for custom REST handlers without UI in openapi.json

Great work on implementing support for custom REST handler definitions in the global configuration! The code is well-organized and includes comprehensive test coverage. The feature will allow users to specify additional REST endpoints not generated by UCC, which will be included in the OpenAPI documentation.

The implementation could benefit from a few minor improvements:

  1. In user_defined_rest_handlers.py, consider adding docstrings to the classes RestHandlerConfig and UserDefinedRestHandlers explaining their purpose and usage.

  2. In schema.json, the pattern for name and endpoint fields could be more descriptive, e.g., "^[a-zA-Z][a-zA-Z0-9_-]*$" to ensure they start with a letter.

  3. In rest_builder/user_defined_rest_handlers.py, consider moving the EAI_DEFAULT_PARAMETERS and EAI_DEFAULT_PARAMETERS_SPECIFIED constants to a separate configuration file since they might need to be modified in the future.

  4. In openapi_generator/ucc_to_oas.py, add docstring for the new function __add_user_defined_paths to explain its purpose and parameters.

  5. In global_config.py, consider adding a docstring for the parse_user_defined_handlers method explaining when it should be called and what it does.

Overall, this appears to be a solid implementation ready for merging after addressing these minor documentation improvements. The test coverage is particularly impressive, with both unit tests and integration tests verifying the functionality.

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:

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