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

Improve values schema #139

Merged
merged 10 commits into from
Mar 5, 2022

Conversation

TylerHelmuth
Copy link
Member

Works towards #135

This PR defines all the properties for opentelemetry-collector's values.schema.yaml. Actual validations to follow.

@TylerHelmuth TylerHelmuth marked this pull request as ready for review March 4, 2022 18:28
@TylerHelmuth TylerHelmuth requested review from a team March 4, 2022 18:28
@TylerHelmuth
Copy link
Member Author

TylerHelmuth commented Mar 4, 2022

The amount of changes kinda got away from me. Let me know what you think and if I should go an add actual validations to this PR as well or if I should make it smaller instead.

@dmitryax
Copy link
Member

dmitryax commented Mar 4, 2022

Let me know what you think and if I should go an add actual validations to this PR as well or if I should make it smaller instead.

What actual validation do you mean? Isn't it enforced already?

@TylerHelmuth
Copy link
Member Author

TylerHelmuth commented Mar 4, 2022

Let me know what you think and if I should go an add actual validations to this PR as well or if I should make it smaller instead.

What actual validation do you mean? Isn't it enforced already?

The requirement for having at least one of standaloneCollector or agentCollector enabled is already present. I was going to review and see if there were any other scenarios. There may not be.

@TylerHelmuth
Copy link
Member Author

Looking through the only other validation I could come up with was something fancy around standaloneCollector.enabled and podDisruptionBudget/autoscaling, but I think all that matters is that enabled is required by the properties. The actual value of enabled doesn't matter. So I think this PR gets the schema into a good state to validate the current values.yaml.

@dmitryax
Copy link
Member

dmitryax commented Mar 4, 2022

Looking through the only other validation I could come up with was something fancy around standaloneCollector.enabled and podDisruptionBudget/autoscaling, but I think all that matters is that enabled is required by the properties. The actual value of enabled doesn't matter. So I think this PR gets the schema into a good state to validate the current values.yaml.

I don't think the fancy validations are that important to have. My idea is that we need to define all the properties with "additionalProperties": false in order to make sure that any replacements of the existing configuration parameters blocks the chart upgrade. It makes any breaking change at least not silently breaking.

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM

@dmitryax dmitryax merged commit da307f4 into open-telemetry:main Mar 5, 2022
@TylerHelmuth TylerHelmuth deleted the improve-values-schema branch March 5, 2022 03:37
dmitryax added a commit that referenced this pull request Mar 18, 2022
Tyler has been actively involved in this repo recently. The following PR were submitted or reviewed:
- #129
- #131
- #139
- #147
- #151
- #142
- #143
- #148

cc @open-telemetry/helm-approvers
dmitryax added a commit that referenced this pull request Mar 20, 2022
Tyler has been actively involved in this repo recently. The following PR were submitted or reviewed:
- #129
- #131
- #139
- #147
- #151
- #142
- #143
- #148

cc @open-telemetry/helm-approvers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants