[confmap] Mark confmap.strictlyTypedInput as stable
#10793
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10793 +/- ##
==========================================
+ Coverage 91.60% 91.69% +0.09%
==========================================
Files 404 404
Lines 18990 18953 -37
==========================================
- Hits 17395 17379 -16
+ Misses 1235 1216 -19
+ Partials 360 358 -2 ☔ View full report in Codecov by Sentry. |
mx-psi
added
the
release:blocker
songy23
reviewed
Aug 5, 2024
TylerHelmuth
approved these changes
Aug 7, 2024
mx-psi
removed
the
release:blocker
|
Discussed on the stability meeting from 2024-08-07, we can have this for v0.108.0 instead of v0.107.0 |
|
@mx-psi ready for a rebase |
mx-psi
force-pushed
the
mx-psi/strict-typing-stable
branch
from
cd787dd
to
7325c66
Compare
dmitryax
approved these changes
Aug 14, 2024
|
I've run into an interesting edge case. Investigating now. Please don't merge for now |
codeboten
reviewed
Aug 15, 2024
| err = fmt.Errorf("cannot unmarshal the configuration: %w", err) | ||
|
|
||
| if globalgates.StrictlyTypedInputGate.IsEnabled() { | ||
| var shouldAddCoda bool |
There was a problem hiding this comment.
I'm likely missing context here, but in other places in this PR we've kept the behaviour inside the check if the StrictlyTypedInputGate.IsEnabled and here it's removed, is this correct?
There was a problem hiding this comment.
The coda says:
Hint: Temporarily restore the previous behavior by disabling
the `confmap.strictlyTypedInput` feature gate. More details at:
https://github.com/open-telemetry/opentelemetry-collector/issues/10552
which is not actionable after this PR is merged (you cannot disable the feature gate). Should we keep some sort of custom message? It could make sense to give people a place where they can complain
👍, if you find something, could you file a separate issue? |
|
I found one issue that should be fixed here #10897 |
|
Last call before I merge this cc @open-telemetry/collector-approvers. I checked the two real issues reported on #10552 and verified that these were fixed with the binary: Can't pass a number to a string fieldIssue: #10552 (comment) (can't pass a number to a Config used to reproduce the issue: Logs with 0.107.0: Logs with HEAD (fails to start but passes validation and unmarshaling): TLS min version in env variableIssue: #10552 (comment) Config used to reproduce the issue: Logs with 0.107.0: Logs with HEAD: I have looked into adding end to end tests, I think we can and should do that, but it will take some time and I don't want to block this and confmap 1.0 on these tests. |
dmitryax
approved these changes
Aug 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Marks
confmap.strictlyTypedInputas stable.Link to tracking issue
Fixes #10552
Blocked by: