-
Notifications
You must be signed in to change notification settings - Fork 56
Deprecate deprecated stability level #607
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
Deprecate deprecated stability level #607
Conversation
|
@open-telemetry/weaver-approvers could I get some eyes on this? |
jerbly
left a comment
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.
This makes sense to me if we're happy to raise the minimum supported version. But, we should have an entry in CHANGELOG.md that states this.
8f8865b to
a449a18
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #607 +/- ##
=====================================
Coverage 74.2% 74.3%
=====================================
Files 57 57
Lines 4578 4595 +17
=====================================
+ Hits 3399 3415 +16
- Misses 1179 1180 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Looks good to me but agree on having a changelog entry for this. |
jsuereth
left a comment
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.
Approval contingent on changelog.
jerbly
left a comment
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.
LGTM
c227070 to
ca8555d
Compare
Semconv don't use
stability: deprecatedfor 1+ years (since version 1.24.0) - open-telemetry/semantic-conventions#588. This is necessary to know if deprecated attribute was stable when it was deprecated (and therefore must be generated in semconv artifact) or it was experimental and does not have to be generated.Having it in weaver is not necessary and misleading. Let's remove it.