-
Notifications
You must be signed in to change notification settings - Fork 48
Conversation
Codecov Report
@@ Coverage Diff @@
## main #70 +/- ##
==========================================
- Coverage 94.99% 94.89% -0.10%
==========================================
Files 41 35 -6
Lines 559 549 -10
Branches 94 93 -1
==========================================
- Hits 531 521 -10
Misses 28 28
Continue to review full report at Codecov.
|
I think we should either close or merge #46 first |
this is missing the review and I think should be a part of this release -> #62 |
@Flarna wants #46 to be merged first. @obecny wants core changes to be made in core before #46 can merge. Those core changes depend on unreleased API changes (we need I fear we are in a dependency loop here. @Flarna @obecny how do you think we should resolve this ordering issue? My plan was to release rc.1 now, make the core changes, release core, then release rc.2 with noops removed. |
If we merge #46 we will have to do some work on core and contrib before people can use it and before we can release core and contrib. What I propose is to wait with merging #46 until it is done first in core and contrib so we don't block ourselves and that we have api that cannot be used with anything. This way we can do release on core and contrib quicker, and we are not blocked. We can do next rc as soon as all necessary changes are done in core and contrib. You merge #46 now and you will have api you cant use with anything for days. unless I'm missing something so pls correct me :) |
I'm fine with this, but I think there might already be some breaks that will need to be implemented in core anyways so it will be blocked no matter what. Holding off on #46 is fine as long as we can get these done quickly. If we merge this now, I'll create a PR to bump API in core immediately so we can make the changes required for #46 |
any opinion here @Flarna? |
If I understand correctly, after you do a new rc api release, we can do next release for core and contrib immediately, nothing needs to be changed except some renaming or using different namespace. All other changes has been done already right ? |
I think most but I'm not 100% sure. |
Will #54 be included as well since it has been merged? |
yes |
reviews pls :) |
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.
apart from #54 missing in changelog lgtm
my intention was to reduce the number of breaking releases as they are usually not that nice for consumers. |
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.
I think the readme should be updated (https://github.com/open-telemetry/opentelemetry-js-api/blob/main/README.md#100-rc0-to-x).
And I assume also the version in package.json needs an update. Or is this done automatically?
OK. Waiting for @obecny because I think all maintainers should agree |
what about this one ? |
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
Tests and lint are passing locally. Is it ok if we merge and release this without the successful actions? |
💥 Breaking Change
span#context()
tospan#spanContext
(@dyladan)🚀 Enhancement
trace#wrapSpanContext
#49 (@dyladan)📝 Documentation
Committers: 6