-
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: make configuration page as optional #1521
base: develop
Are you sure you want to change the base?
Conversation
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.
seems good just one minor thing that configuration page is technically handled, so there isn't any entry point but if you directly type url it displays empty page. We can handle it in separate PR
Signed-off-by: Viktor Tsvetkov <[email protected]>
...expected_addon_no_configuration/Splunk_TA_UCCExample/appserver/static/js/build/entry_page.js
Outdated
Show resolved
Hide resolved
I had checked it in my VM, when we add |
Ok, then it has to be caching on my side, i will double check but thats a minor problem as there shouldn't be any access point to this page anyway. |
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 configuration, inputs and dashboard pages are optional - should we make pages optional as well? So that we can support
EDIT: if we decide so - it deserves a separate small PR. |
Yes, we will be covering that in |
@@ -40,14 +43,20 @@ def generate_nav_default_xml( | |||
""" | |||
nav = ET.Element("nav") | |||
if include_inputs: | |||
if default_view == "inputs": | |||
if ( | |||
not (include_configuration) and default_view == "configuration" |
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.
not (include_configuration) and default_view == "configuration"
This is a strange condition. It would be good to change _validate_meta_default_view
method in global_config_validator.py
to not allow this value when there is no configuration page and change this condition.
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 agree with Kamil. _validate_meta_default_view
already checks, for input
and dashboard
, if default view is present in the global config and raises an error if not, so in my opinion it would be the best to expand it to configuration
as well.
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.
Have updated the logic and added validation in _validate_meta_default_view
function
@@ -40,14 +43,20 @@ def generate_nav_default_xml( | |||
""" | |||
nav = ET.Element("nav") | |||
if include_inputs: | |||
if default_view == "inputs": | |||
if ( | |||
not (include_configuration) and default_view == "configuration" |
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 agree with Kamil. _validate_meta_default_view
already checks, for input
and dashboard
, if default view is present in the global config and raises an error if not, so in my opinion it would be the best to expand it to configuration
as well.
splunk_add_on_ucc_framework/generators/xml_files/create_configuration_xml.py
Show resolved
Hide resolved
) | ||
if default_view == "configuration" and self._global_config.has_configuration(): |
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 default_view == "configuration" and self._global_config.has_configuration(): | |
if default_view == "configuration" and not self._global_config.has_configuration(): | |
```? |
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 catch! Updated the condition.
splunk_add_on_ucc_framework/commands/rest_builder/global_config_builder_schema.py
Outdated
Show resolved
Hide resolved
splunk_add_on_ucc_framework/generators/html_files/create_alert_actions_html.py
Show resolved
Hide resolved
return [ | ||
resolve_tab(tab) for tab in self._config["pages"]["configuration"]["tabs"] | ||
] | ||
def config_tabs(self) -> List[Any]: |
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 this be a part of a globalConfig
class logic?
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 could resolve tabs if there are tabs in the globalConfig
file, otherwise return an empty list. This way the validation logic below could be simplified and if pages.get("configuration")
statements could be removed.
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.
globalConfig class already has a similar tabs
property (part of v5.30.0) and this method was existing since v5.40.0.
I think I'll have to merge, clear and clean-up both the functions and its tests. Making these changes as a part of this PR would be it make it bigger and would have two unrelated changes.
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.
Ok, agree, please create a ticket for it and let's refactor it after this PR is merged.
...ddon_no_configuration/Splunk_TA_UCCExample/appserver/static/js/build/entry_page.licenses.txt
Outdated
Show resolved
Hide resolved
...expected_addon_no_configuration/Splunk_TA_UCCExample/appserver/static/js/build/entry_page.js
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,176 @@ | |||
{ | |||
"pages": { | |||
"inputs": { |
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.
That's a great example - should we require configuration
page if inputs
page is present?
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.
No, I can think of a use case for add-ons like Nix and Windows, where the shell/batch scripts are present, and all users have to do is enable the inputs. Similarly, there could be other such add-on use cases where configuration page may not be required.
for tab in self.tabs: | ||
if "table" in tab: | ||
configs.append(tab) | ||
if self.has_configuration(): |
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.
self.tabs
would be an empty list if there is no configuration, we can remove this if
statement as well.
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.
Made the changes.
@@ -34,15 +33,15 @@ def generate_nav_default_xml( | |||
include_inputs: bool, | |||
include_dashboard: bool, | |||
include_configuration: bool, | |||
default_view: str, | |||
default_view: Union[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.
I was wondering about this syntax, I found https://google.github.io/styleguide/pyguide.html#3195-nonetype
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.
Made the changes.
return [ | ||
resolve_tab(tab) for tab in self._config["pages"]["configuration"]["tabs"] | ||
] | ||
def config_tabs(self) -> List[Any]: |
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.
Ok, agree, please create a ticket for it and let's refactor it after this PR is merged.
_collapse_entities( | ||
global_config.content["pages"]["configuration"].get("tabs"), entity_type | ||
) | ||
if global_config.has_configuration(): |
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.
_collapse_entities
does accept None
as a first option and it immediately returns, we can remove this if
statement.
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.
Made the changes.
Issue number:
ADDON-76819
PR Type
What kind of change does this PR introduce?
Summary
Set configuration as an optional key in
globalConfig.json
.Changes
Made configuration page optional in
schema.json
, So that users can have an add-on UI without Configuration page.Also added a smoke testcase which captures this behaviour.
User experience
Now, user can build their add-on without specifying
configuration
in theirglobalConfig.json
. Users now can have a UI without Configuration page for their add-on.Checklist
If an item doesn't apply to your changes, leave it unchecked.