-
Notifications
You must be signed in to change notification settings - Fork 144
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
Add a Kubernetes Distribution #507
Add a Kubernetes Distribution #507
Conversation
c24535a
to
7322e3f
Compare
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 set of components LGTM assuming we remove the deprecated logging exporter
/cc @open-telemetry/operator-approvers |
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.
It would be great to have the operator folks review this as well.
@avillela , you had a list of components during your talk yesterday and while I believe they are all here already, I'd appreciate it if you could double check.
a9ee46d
to
96b84a9
Compare
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.
Thank you for making this work, @TylerHelmuth !
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.
🚀
Are you referring to the Collector components for k8s @jpkrohling? |
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.
My general comment is "let's start smaller", by releasing for fewer OSs, architectures and types of artifacts
@mx-psi @swiatekm-sumo you're comments on reducing the produced artifacts is valid, and annoying lol I will play around with what changes are necessary in |
3da3431
to
e3a6b88
Compare
e3a6b88
to
bd05cce
Compare
@@ -13,3 +13,5 @@ | |||
# | |||
|
|||
* @open-telemetry/collector-contrib-approvers | |||
|
|||
distributions/otelcol-k8s/ @open-telemetry/collector-contrib-approvers @open-telemetry/helm-approvers @open-telemetry/operator-approvers |
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.
Shouldn't here be as well the operator maintainers?
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.
Doesn't @open-telemetry/operator-approvers include the maintainers?
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.
Correct. I went with -approvers
instead of maintainers because thats how we do our own code owners file and its how the otel site handles adding other teams as code owners. In my opinion helm and operator Approvers are totally qualified to review PRs for this distro.
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.
@TylerHelmuth is there anything we need to do to resolve the error message github is giving us here?
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.
Ya, we need to give those groups access to this repo. I'll open a community issue for that.
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.
Created open-telemetry/community#2039
@open-telemetry/helm-approvers please review |
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.
LGTM
Shouldn't we add this distro to the main README.md file?
|
@astencel-sumo updated the readme and removed some statements that are no longer universally true. |
Ran another test against the latest commits in this branch (and 1 extra commit to update the references to my repo/dockerhub for the artifacts:
Artifacts: https://github.com/TylerHelmuth/opentelemetry-collector-releases/releases/tag/v0.97.9 Tested locally using the values.yamls from the description with the updated tag and can see the collector running as expected. |
🎉 |
This PR adds a new distribution specifically for monitoring Kubernetes. For full details see #357.
Tested in https://github.com/TylerHelmuth/opentelemetry-collector-releases:
Example helm chart values.yaml using this distro:
Closes #357