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

chore(aggregators): Comment out default values in sample configs #15864

Conversation

Hipska
Copy link
Contributor

@Hipska Hipska commented Sep 10, 2024

Summary

  • Aligns all aggregator sample configs so they have commented out defaults for the common parameters.
  • Adds a test to validate the sample config

Checklist

  • No AI generated code was used in this PR

Related issues

@telegraf-tiger telegraf-tiger bot added the chore label Sep 10, 2024
@Hipska Hipska changed the title chore(aggregators): Comment out default values and validate sample config in tests chore(aggregators): Comment out default values and validate sample configs Sep 10, 2024
@Hipska
Copy link
Contributor Author

Hipska commented Sep 10, 2024

!retry-failed

@srebhan
Copy link
Member

srebhan commented Sep 10, 2024

@Hipska please do not add the tests here. I would suggest to add a test to config/config_test.go iterating over all known plugins and do this. Maybe one test per plugin category would be the best. This way we do have the test in one place and do not need to stray the code across all plugins...

@Hipska
Copy link
Contributor Author

Hipska commented Sep 10, 2024

We discussed this before when I introduced the DefaultSampleConfig testutil. I wanted to do something like that, but you were opposed to that since some plugins have special handling of the sample config and thus cannot be tested like you just requested..

@srebhan
Copy link
Member

srebhan commented Sep 11, 2024

I said we should be careful with the test and add a whitelist for plugins we know are different.

@Hipska
Copy link
Contributor Author

Hipska commented Sep 11, 2024

So, next actions: Remove the tests?

@srebhan
Copy link
Member

srebhan commented Sep 11, 2024

Yes.

@Hipska Hipska changed the title chore(aggregators): Comment out default values and validate sample configs chore(aggregators): Comment out default values in sample configs Sep 11, 2024
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Hipska!

@srebhan srebhan added ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. plugin/aggregator 1. Request for new aggregator plugins 2. Issues/PRs that are related to aggregator plugins labels Sep 12, 2024
@DStrand1 DStrand1 merged commit dd94512 into influxdata:master Sep 12, 2024
31 checks passed
@github-actions github-actions bot added this to the v1.32.1 milestone Sep 12, 2024
@Hipska Hipska deleted the chore/aggregators/sample_config_defaults branch September 13, 2024 07:56
srebhan pushed a commit that referenced this pull request Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore plugin/aggregator 1. Request for new aggregator plugins 2. Issues/PRs that are related to aggregator plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants