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.

Sign up for GitHub

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

Jump to bottom

update deps to 0.42.0 and provide double dollar config source parsing compatibility #1099

Merged
merged 2 commits into from
Jan 13, 2022
Merged

update deps to 0.42.0 and provide double dollar config source parsing compatibility #1099

merged 2 commits into from
Jan 13, 2022

Conversation

rmfitzpatrick
Copy link
Contributor

initial adoption of config (map) provider helpers (open-telemetry/opentelemetry-collector#4637) to address breaking changes (open-telemetry/opentelemetry-collector#4600).

Probably shouldn't land until it is known to accomplish what #1058 does and has been vetted w/ helm chart and additional integration test(s).

@rmfitzpatrick rmfitzpatrick requested review from a team as code owners January 10, 2022 18:17
@rmfitzpatrick rmfitzpatrick force-pushed the adopt0420 branch 2 times, most recently from acf3454 to 496899d Compare January 10, 2022 18:47
@rmfitzpatrick
Copy link
Contributor Author

@pmcollins do you think adding the ReplaceDollarDollar config converter to these changes will be adequate for $$ fix and backward compatibility?

@pmcollins
Copy link
Contributor

pmcollins commented Jan 10, 2022 •
edited
Loading

Sure, if that bug is fixed, let's do it and consider adding the relevant tests from that PR as well (or their equivalent).

BTW the Koanf API we were using for fixing up configs doesn't handle arrays so I had to put in a bit of a hack to replace values that live under arrays:

https://github.com/signalfx/splunk-otel-collector/pull/1058/files#diff-c442615bff17bb0e7926d1f617ad467323a38da736f4a30341d427301023cd66R42

dmitryax
dmitryax approved these changes Jan 11, 2022
View reviewed changes
Copy link
Contributor

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM. I can confirm it fixes the double evaluation issue #628

@dmitryax
Copy link
Contributor

@rmfitzpatrick @pmcollins . I'm still not sure if we want to have the automatic translation / backward compatibility for $$ -> $. It doesn't seem right to me. Let's discuss it first

@rmfitzpatrick rmfitzpatrick mentioned this pull request Jan 11, 2022
Merged
@dmitryax dmitryax mentioned this pull request Jan 12, 2022
Closed
@rmfitzpatrick rmfitzpatrick requested a review from a team as a code owner January 12, 2022 21:04
@rmfitzpatrick
Copy link
Contributor Author

@dmitryax, @pmcollins as discussed these changes now include a temporary $${config_source:value} -> ${config_source:value} converter. This must occur in the manager since config converter function invocation occurs after the map is retrieved from the config map providers.

@rmfitzpatrick rmfitzpatrick changed the title DRAFT: update deps to 0.42.0 DRAFT: update deps to 0.42.0 and provide double dollar config source parsing compatibility Jan 12, 2022
@rmfitzpatrick rmfitzpatrick force-pushed the adopt0420 branch 3 times, most recently from 436f4c3 to d5dec87 Compare January 12, 2022 21:48
@rmfitzpatrick rmfitzpatrick changed the title DRAFT: update deps to 0.42.0 and provide double dollar config source parsing compatibility update deps to 0.42.0 and provide double dollar config source parsing compatibility Jan 12, 2022
@rmfitzpatrick
Copy link
Contributor Author

@dmitryax any chance you'd be able to weigh in on some suggested upgrade guidelines re: #628 ?

dmitryax
dmitryax reviewed Jan 13, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
internal/configprovider/manager.go Outdated Show resolved Hide resolved
tests/general/testdata/resource_metrics/envvar_labels.yaml Show resolved Hide resolved
@dmitryax
Copy link
Contributor

@dmitryax any chance you'd be able to weigh in on some suggested upgrade guidelines re: #628 ?

Sure I can add an upgrade guideline once this PR is merged.

@rmfitzpatrick rmfitzpatrick force-pushed the adopt0420 branch 4 times, most recently from dd48eed to ed1bddc Compare January 13, 2022 17:27
dmitryax
dmitryax approved these changes Jan 13, 2022
View reviewed changes
Copy link
Contributor

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM!

CHANGELOG.md Outdated Show resolved Hide resolved
@rmfitzpatrick rmfitzpatrick merged commit 91f554c into main Jan 13, 2022
@delete-merged-branch delete-merged-branch bot deleted the adopt0420 branch January 13, 2022 18:27
@dmitryax dmitryax mentioned this pull request Jan 14, 2022
Merged
@dmitryax dmitryax mentioned this pull request Jul 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitryax dmitryax dmitryax approved these changes

@pmcollins pmcollins Awaiting requested review from pmcollins

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rmfitzpatrick @pmcollins @dmitryax