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] Deprecate expandconverter #10510

Merged
merged 16 commits into from
Aug 7, 2024

Conversation

TylerHelmuth
Copy link
Member

Description

This PR deprecates expandconverter and removes its use from otelcoltest.LoadConfig and OCB.

This cannot be merged until the confmap.unifyEnvVarExpansion feature gate is made stable.

Link to tracking issue

closes #10161
closes #7111
closes #8215

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.64%. Comparing base (b688e7a) to head (80b3e16).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10510      +/-   ##
==========================================
+ Coverage   91.37%   91.64%   +0.26%     
==========================================
  Files         406      406              
  Lines       19007    18998       -9     
==========================================
+ Hits        17368    17411      +43     
+ Misses       1282     1227      -55     
- Partials      357      360       +3     

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

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@dmitryax
Copy link
Member

The same comment here as in #10723. We need to move the $$ escaping from the converter to the resolver and ensure ${FOO} expansion without the converter.

@TylerHelmuth
Copy link
Member Author

@dmitryax I'll handle that work as part of #10508

@mx-psi mx-psi added the release:blocker The issue must be resolved before cutting the next release label Aug 7, 2024
@TylerHelmuth TylerHelmuth marked this pull request as ready for review August 7, 2024 16:16
@TylerHelmuth TylerHelmuth requested review from a team and songy23 August 7, 2024 16:16
confmap/converter/expandconverter/expand.go Outdated Show resolved Hide resolved
confmap/converter/expandconverter/expand.go Outdated Show resolved Hide resolved
confmap/converter/expandconverter/go.mod Outdated Show resolved Hide resolved
@TylerHelmuth
Copy link
Member Author

The failed test will be handled by #10508.

dmitryax pushed a commit that referenced this pull request Aug 7, 2024
…0508)

#### Description

This PR promotes the `confmap.unifyEnvVarExpansion` feature gate to
stable and sets a `ToVersion` of `v0.106.0`, anticipating that the gate
be completely removed in that version.

We should weigh if switching the Stable should be done in `v0.105.0` or
if it needs more time in `Beta` to give users more time to switch.
Delaying promotion to `Stable` delays confmap 1.0.

If we merge this we need to commit to merging
#10510 in
the same release.

#### Link to tracking issue
Related to
#10161
Related to
#7111
Related to
#8215

---------

Co-authored-by: Evan Bradley <[email protected]>
@dmitryax
Copy link
Member

dmitryax commented Aug 7, 2024

Merged #10508. Please rebase

# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: [api]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
change_logs: [api]
change_logs: [user]

I'm tempted to say that we should include this in the user-facing changelog since we encourage users to build their own Collector binaries.

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree. The previous PR handled the user-facing changelog. This PR (which could have be in #10508 and was only separated for size), removes the expandconverter from the ocb's generated main.go, but end users don't need to worry about that.

Copy link
Member

Choose a reason for hiding this comment

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

We need to define the end user here :) I'm ok with any. We struggle on the same Q for madatagen "users".

Copy link
Member

@dmitryax dmitryax Aug 7, 2024

Choose a reason for hiding this comment

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

Potentially, we can have 2 more changelogs: ocb and mdatagen

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe user denotes those who run the collector and api denotes those who import libraries into their code.

Copy link
Member

Choose a reason for hiding this comment

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

I'm merging as is for now. Feel free to continue the discussion and we can change this line if needed

@dmitryax dmitryax merged commit 2b9697f into open-telemetry:main Aug 7, 2024
50 checks passed
@github-actions github-actions bot added this to the next release milestone Aug 7, 2024
@Erog38
Copy link

Erog38 commented Oct 24, 2024

Hey, bumping this because removing this expansion seems to have broken the capability for YAML to have predefined chunks which can be re-used across multi-organization support teams. I'll make an issue and try to repro, but the tl;dr is that we used to do something like this:

...
filelog:
  <<: ${internal:filelog-defaults}
  include:
  - some additional include

This use to work as a way to allow users to merge predefined configuration with their own overrides and no longer functions. I'm fairly confident it's due to this removal. Is there any hope to address this issue or is it just an unsupported feature?

@mx-psi
Copy link
Member

mx-psi commented Oct 24, 2024

@Erog38 Please file an issue with a repro and we can take a look :)

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
8 participants