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: extended rh_templates to pass user validators to resthandler #1343

Closed
wants to merge 4 commits into from

Conversation

sgoral-splunk
Copy link
Contributor

@sgoral-splunk sgoral-splunk commented Sep 16, 2024

Issue number:ADDON-74237

Summary

Extended rh_templates to pass user validators to resthandler
this pr requires changes from splunktauuclib: splunk/addonfactory-ucc-library#316

Changes

  • rh_templates now has an additional list "special_fields"
  • rh_templates now adds "special_fields" to RestModel object
  • GlobalConfigBuilderSchema now calculates "special_fields" for rh_templates

User experience

Name field will be validated with users validators, provided in the globalconfig, on the server side.

Checklist

If your change doesn't seem to apply, please leave them unchecked.

@sgoral-splunk
Copy link
Contributor Author

This PR requires splunktaucclib in version 6.4 with the following changes:
splunk/addonfactory-ucc-library#316

@sgoral-splunk sgoral-splunk changed the title feat: extended rh_templates to pass custom validators to splunktauccl… feat: extended rh_templates to pass user validators to resthandler Sep 17, 2024
@sgoral-splunk sgoral-splunk marked this pull request as ready for review September 17, 2024 08:23
@sgoral-splunk sgoral-splunk requested review from a team as code owners September 17, 2024 08:23
for field in fields_content:
if field["field"] != "name":
fields.append(
RestFieldBuilder(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull building object up to for loop

@@ -33,10 +33,14 @@
class RestEntityBuilder:
_title_template = "[{}]"
_rh_template = """
special_fields = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If special_fields are not present, can we omit this variable declaration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can, but how do we benefit from it? I think it's not worth adding extra logic and template just to avoid declaring an empty list. What do you think about it?

self,
name: Optional[str],
fields: List["RestFieldBuilder"],
special_fields: List["RestFieldBuilder"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In splunktaucclib you had made this parameter of type Optional[List[RestField]] should we mention the same thing here.

for field in self._fields:
field_line = field.generate_rh()
fields.append(field_line)
for special_field in self._special_fields:
field_line = special_field.generate_rh()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think here we can use list comprehension, or we can remove redundant variable declaration.

@@ -14,6 +14,10 @@
util.remove_http_proxy_env_vars()


special_fields = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we skip the declaration of special_fields if it is an empty list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like above: "we can, but how do we benefit from it? I think it's not worth adding extra logic and template just to avoid declaring an empty list. What do you think about it?"

@github-actions github-actions bot locked and limited conversation to collaborators Oct 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants