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

[confmap] Store original string in confmap.Conf #10618

Merged

Conversation

mx-psi
Copy link
Member

@mx-psi mx-psi commented Jul 15, 2024

Description

  • Adds new expandedValue struct that holds the original string representation if available for values resolved from a provider.
  • Removes any mention of expandedValue in the public API by adding a sanitize step before returning any Gets or ToStringMaps.
  • Adds new decoding hook that checks if the target field is of string type and uses the string representation in that case.

Link to tracking issue

Fixes #10605, Fixes #10405, Fixes #10659

Testing

This changes the behavior in some cases, I update the test cases.

Documentation

ENV value ${ENV} before unification ${ENV} in v0.105.0 (also ${env:ENV} before unification) Value after this PR
foo\nbar foo\nbar foo bar foo\nbar
1111:1111:1111:1111:1111:: 1111:1111:1111:1111:1111:: Error 1111:1111:1111:1111:1111::
"0123" "0123" 0123 "0123"

@mx-psi mx-psi force-pushed the mx-psi/string-value-for-string-fields branch 2 times, most recently from 9d5c9ad to fbbfc01 Compare July 16, 2024 11:16
@mx-psi mx-psi changed the title Store original string in confmap.Conf [confmap] Store original string in confmap.Conf Jul 16, 2024
@mx-psi mx-psi force-pushed the mx-psi/string-value-for-string-fields branch from fbbfc01 to 4f0a7ff Compare July 16, 2024 11:21
@mx-psi mx-psi marked this pull request as ready for review July 16, 2024 11:21
@mx-psi mx-psi requested review from a team and TylerHelmuth July 16, 2024 11:21
@mx-psi
Copy link
Member Author

mx-psi commented Jul 16, 2024

I still need to think about some edge cases and add more tests, but the overall approach should be clear now and can be reviewed

Copy link

codecov bot commented Jul 16, 2024

Codecov Report

Attention: Patch coverage is 85.13514% with 11 lines in your changes missing coverage. Please review.

Project coverage is 92.35%. Comparing base (7d5b1ba) to head (589a45f).

Files Patch % Lines
confmap/expand.go 65.38% 7 Missing and 2 partials ⚠️
confmap/confmap.go 95.34% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10618      +/-   ##
==========================================
- Coverage   92.38%   92.35%   -0.04%     
==========================================
  Files         403      403              
  Lines       18729    18796      +67     
==========================================
+ Hits        17303    17359      +56     
- Misses       1066     1074       +8     
- Partials      360      363       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

confmap/expand.go Outdated Show resolved Hide resolved
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

I think this approach makes sense to address the issue, there are some lint errors that need addressing

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

I agree that this looks like the right approach to me as it lets users continue to set yaml via env vars but also use exact inline env var values in strings.

@mx-psi
Copy link
Member Author

mx-psi commented Jul 19, 2024

@MovieStoreGuy it would really help if you can try this one out and confirm that it fixes your issue in an environment as close as possible to the one where you found it

@mx-psi mx-psi added the release:blocker The issue must be resolved before cutting the next release label Jul 19, 2024
@mx-psi
Copy link
Member Author

mx-psi commented Jul 22, 2024

Final call for reviews, I want to merge this by Thursday EU morning cc @open-telemetry/collector-approvers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:blocker The issue must be resolved before cutting the next release
Projects
None yet
3 participants