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: generate another .conf-file when conf parameter is used #1608

Open
wants to merge 23 commits into
base: develop
Choose a base branch
from

Conversation

kkedziak-splunk
Copy link
Contributor

@kkedziak-splunk kkedziak-splunk commented Feb 28, 2025

Issue number: ADDON-78536

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

The "conf" parameter for inputs indicates that a separate config file should be used. Therefore inputs with this parameter should not be included in inputs.conf and inputs.conf.spec.

Also, disableNewInputs parameter is improved.

User experience

No changes.

Checklist

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

@kkedziak-splunk kkedziak-splunk requested a review from a team as a code owner February 28, 2025 14:44
@hetangmodi-crest
Copy link
Contributor

Should we create .conf.spec for the inputs that have the "conf" parameter defined?

@kkedziak-splunk
Copy link
Contributor Author

@hetangmodi-crest
Do you mean placing placeholder = placeholder as it used to be, or the actual parameters?

@hetangmodi-crest
Copy link
Contributor

I mean the actual parameters with their description and default values (if provided). Just like we are having inputs.conf.spec, we should have the same for services that have the conf parameter defined so that Splunkd doesn't complain about invalid conf files on restarts.

@artemrys
Copy link
Member

artemrys commented Mar 5, 2025

I mean the actual parameters with their description and default values (if provided). Just like we are having inputs.conf.spec, we should have the same for services that have the conf parameter defined so that Splunkd doesn't complain about invalid conf files on restarts.

@kkedziak-splunk you can check one of our TAs to see the example, one of the cloud TAs should be a good candidiate.

@pull-request-size pull-request-size bot added size/L and removed size/M labels Mar 11, 2025
@@ -80,7 +80,7 @@ def _set_attributes(self, **kwargs: Any) -> None:
value = str(field_default_value)

default_values[entity["field"]] = value
field_value_parts.append(f"Default: {field_default_value}")
field_value_parts.append(f"(Default: {value})")
Copy link
Member

Choose a reason for hiding this comment

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

Can you please shed more light on this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks like a lot of changes but this is similar to what was before, but slightly rewritten and with very small changes. For example, I changed the variables, as some names meant something else, plus I changed one if to remove one level of indentation.

The flow:
If global config is not specified, return (the same as above, but I negated the "if" to remove the indentation)
For every service:

  1. Setting spec_properties. If we don't have a "conf" parameter, the structure that stores spec properties for inputs.conf is used. Otherwise, we get a structure for a separate config (this is a change, as previously we only saved everything in inputs.conf, even conf services).
  2. Handling disableNewInput - not changed.
  3. Previously we did not iterate the entities if there was the "conf" parameter - this is removed now
  4. For every entity in the service:
    4.1. Skip if field name == "name" (not changed)
    4.2. Skip if field type == "helpLink" (added - inspired by openapi.json)
    4.3. Handling the defaultValue. This already happened but I moved it up, because there were situations (not tested), where if we had a bool value, the inputs.conf had e.g. field = true, while inputs.conf.spec had Default: True (first letter was upper case). I also added a test for this branch.
    4.4. The spec line is generated as follows: {field_name} = {field_help} (default: {default_value}) - small change to what was before, can be seen in tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

This explanation as code comments would help much more than on the PR.

@artemrys
Copy link
Member

@hetangmodi-crest would appreciate your review on this change please :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we also add the custom .conf.spec file in this expected folder? In our example, I think it would splunk_ta_uccexample_service_with_conf_param.conf.spec, as the actual content where the inputs would be written is splunk_ta_uccexample_service_with_conf_param.conf. The name would be the same as the one stored REST handler scripts.

@@ -80,7 +80,7 @@ def _set_attributes(self, **kwargs: Any) -> None:
value = str(field_default_value)

default_values[entity["field"]] = value
field_value_parts.append(f"Default: {field_default_value}")
field_value_parts.append(f"(Default: {value})")
Copy link
Contributor

Choose a reason for hiding this comment

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

This explanation as code comments would help much more than on the PR.

@artemrys artemrys changed the title feat: do not put "conf" inputs in inputs.conf feat: generate another .conf-file when conf parameter is used Mar 28, 2025
Copy link
Member

Choose a reason for hiding this comment

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

For me to understand - we do not have service_with_conf_param in inputs.conf anymore but we did not create a dedicated file for it during the build time.

Did we check that this file will be indeed created when a new input is created?

Can we please also check whether AWS add-on is not impacted because of this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I was experimenting and decided to revert this part and add entries to inputs.conf for every input, as this parameter python.version = python3, although not read by the input, it may be read by Splunk when starting the input.
  2. Yes, the file is created when input is added. I tested it locally.
  3. AWS shows no changes (used diff -r dir1 dir2) but it is because it contains those files in the default directory. The same is with the GCP TA.

One more thing about conf files not being created - I was experimenting with that and it seems that we could create those files with default values. But on the other hand we haven't done that so far. And I saw that default values are handled by the handlers.

# Conflicts:
#	splunk_add_on_ucc_framework/generators/conf_files/create_inputs_conf.py
#	tests/unit/generators/conf_files/test_create_inputs_conf.py
Copy link

github-actions bot commented Apr 3, 2025

Code Coverage 🎉

Type PR Develop Change Status
Line Coverage 87.41% 87.33% 0.08% 🟢 Increased
Branch Coverage 80.65% 80.53% 0.11% 🟢 Increased

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.

3 participants