Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update operator integration statuses and cli arg options #1251
Update operator integration statuses and cli arg options #1251
Changes from all commits
b99a3d8
2df16a4
cfbfd92
27ca4dc
b81b9a8
e4a19e5
40244e3
e00d4a9
128f36e
e4b4506
3b78d27
9576dd3
132ad4e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What extra properties do we supply?
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.
The upstream operator chart doesn't have a field to enable adding default instrumentation anymore since open-telemetry/opentelemetry-helm-charts#1065 was added. This affected us and other companies.
instrumentation.*
values fails validation testsThere 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.
Which field in
instrumentation.*
?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 cannot find
instrumentation
section in https://github.com/open-telemetry/opentelemetry-helm-charts/blob/main/charts/opentelemetry-operator/values.yaml. Is this only used on our side?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.
We should've not used a section that is supposed to be passed to the sub-chart for our own purposes. Now we should either:
instrumentation
out fromoperator
breaking users relying on this optionoperator
alias breaking user passing stuff to the operator.The second option seems preferable to me. Similar to what you do, but instead of removing subchart, remove the alias
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 understand. But this is still wrong. open-telemetry/opentelemetry-helm-charts#1065 should not be reverted. There is nothing wrong with that PR
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.
Trying to explain why it's wrong: Imagine the sub-chart adds support of the
instrumentation
section with conflicting behavior.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.
Actually, I think we should move
instrumentation
out ofoperator
and keep the alias. Otherwise we breakoperator.enabled
andoperator.admissionWebhooks
.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.
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.
This ticket is blocked until we complete the following in a separate PR.