-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[configopaque, confmap] Change yaml test by confmap test #9442
[configopaque, confmap] Change yaml test by confmap test #9442
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9442 +/- ##
==========================================
- Coverage 90.25% 90.24% -0.02%
==========================================
Files 344 344
Lines 17932 17932
==========================================
- Hits 16185 16182 -3
- Misses 1419 1421 +2
- Partials 328 329 +1 ☔ View full report in Codecov by Sentry. |
It seems like you both agree on doing it the other way around. I prefer doing it this way but don't feel strongly about it, and we can always change it after the fact, so let me fix it :) |
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.
Honestly the more I think of it, this really ought to be an integration test somewhere else in the repo. What we're really checking is that both the config packages and confmap use the same encoding
interfaces when neither depends on the other.
That said, this looks good to me.
That makes sense to me. I think for now we can go down this route since this is the status quo and changing this is not a breaking change |
Description:
configopaque
Link to tracking Issue: From discussion on #9427