-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[internal/patch/grpc-go-insecure] Add patch to grpc-go dependency #10638
Conversation
617470f
to
95fe859
Compare
Co-authored-by: Albert Vaca Cintora <[email protected]>
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.
Looks good to me, thanks for adding detailed docs on the change. Just a suggestion on an additional comment.
I think we can mention explicitly in the description that this change doesn't require any specific QA on other parts of the agent that use grpc (other than the otel pipeline), given the nature of the change.
|
||
go 1.16 | ||
|
||
// Should match the version we replace on the main go.mod |
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.
should this comment also be mentioned in the top-level go.mod of the repo (next to the line that pins google.golang.org/grpc
?)
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.
Makes sense, I added a comment
Updated the QA instructions to mention only OTLP |
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.
Amazing, thank you.
@@ -0,0 +1,27 @@ | |||
# `google.golang.org/grpc/credentials/insecure` patch | |||
|
|||
## What is this? |
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 does this PR do?
Patches
grpc-go
dependency to addinsecure
package.Motivation
Since open-telemetry/opentelemetry-collector#4586, the
go.opentelemetry.io/collector
dependency requires theinsecure
grpc-go package. Since we can't bumpgrpc-go
due to etcd-io/etcd#12124, and alternative solutions like bumping Kubernetes are not doable either (see kubernetes/kubernetes#106536), we patch grpc-go through areplace
directive to be able to bumpgo.opentelemetry.io/collector
to the latest version.Additional Notes
See more detailed explanation on the
README.md
file.Possible Drawbacks / Trade-offs
We are patching an important dependency, which is risky. However, we are only adding new code, and this code is copied verbatim from a future version of the dependency.
Describe how to test/QA your changes
It should be a no-op. Since this new package will only be imported by the OpenTelemetry Collector dependency, we can QA by checking the OTLP gRPC endpoint. Other gRPC features need no testing.
Reviewer's Checklist
Triage
milestone is set.team/..
label has been applied, if known.major_change
label if your change either has a major impact on the code base, is impacting multiple teams or is changing important well-established internals of the Agent. This label will be use during QA to make sure each team pay extra attention to the changed behavior. For any customer facing change use a releasenote.changelog/no-changelog
label has been applied.qa/skip-qa
label is not applied.need-change/operator
andneed-change/helm
labels have been applied.