-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[chore]: bump prometheus common dep to support RFC7523 3.1 #44381
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
Conversation
|
Welcome, contributor! Thank you for your contribution to opentelemetry-collector-contrib. Important reminders:
A maintainer will review your pull request soon. Thank you for helping make OpenTelemetry better! |
|
Tomorrow I'll check the unit tests to solve them. I think that after latest changes testing I have broken them |
|
Could you trigger again tests please? I've found the issue and I've fixed it. Current Instead of allowing *config.Secret (my first option) as custom marshaler config, we just need to omit empty values (changing the custom marshaller breaks the "magic" to not corrupt the value with |
|
I've found that there is a problem with the omit empty for |
e213bfe to
5f8e057
Compare
Signed-off-by: Jorge Turrado <[email protected]>
5f8e057 to
a5018eb
Compare
|
This automated PR updated the pkg to 0.67.3 which technically adds the support, but the version 0.67.3 didn't work for otel collector because a marshal problem that happened with Does it make sense to create a changelog record to notify that this support has been added as as enhancement? This comes just from bumping deps but maybe it's relevant for someone |
|
Thanks! I wouldn't call this "chore" though, since it adds a feature. Could you add a changelog entry? |
Sure, I'm going to review the CI and add the changelog. Thanks! |
|
Reading https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md I have it clear now, sorry because I should have read it before :( |
Signed-off-by: Jorge Turrado <[email protected]>
|
@ArthurSens could you trigger again the CI? |
|
lol, when I've consolidated dependencies |
ArthurSens
left a comment
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!
No worries about pinging several people, the codebase of this repository is organized like this and bumping a widely used dependency will always ping this many people 😅
|
Thank you for your contribution @JorTurFer! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. If you are getting started contributing, you can also join the CNCF Slack channel #opentelemetry-new-contributors to ask for guidance and get help. |
Description
Prometheus common pkg supports JWT Profile for Oauth (RFC7523 3.1) since latest version (v0.67.3). This PR bumps the dependency and also update the custom yaml parser to handle that now there are more than 1 commonconfig.Secret and one or more it can be empty.
Testing
I've tested this using the kong integration test, just updating the file with the values required to invoke the new code from prometheus common pkg and checked that I can get a valid token from the oauth server that requires this grant-type
I don't know if this requires some extra test (maybe some unit/integration) as this really comes as feature of prometheus dep and it's already tested there